Skip to content

feat(platform-wallet): add platform-wallet-storage crate (sqlite persister)#3625

Open
Claudius-Maginificent wants to merge 67 commits into
v3.1-devfrom
feat/platform-wallet-sqlite-persistor
Open

feat(platform-wallet): add platform-wallet-storage crate (sqlite persister)#3625
Claudius-Maginificent wants to merge 67 commits into
v3.1-devfrom
feat/platform-wallet-sqlite-persistor

Conversation

@Claudius-Maginificent
Copy link
Copy Markdown
Collaborator

@Claudius-Maginificent Claudius-Maginificent commented May 11, 2026

Issue being fixed or feature implemented

platform-wallet's PlatformWalletPersistence trait (packages/rs-platform-wallet/src/changeset/traits.rs:118) shipped with only NoPlatformPersistence — a no-op stub. The canonical SQLite implementation lived downstream inside dash-evo-tool and was not reusable by FFI consumers, desktop apps, tests, or CLI tooling. This PR introduces a new workspace crate, platform-wallet-storage, that owns the canonical SQLite persister today and reserves a submodule slot for a future SecretStore (OS keyring + encrypted file backends) so storage concerns for the wallet stack live under one roof.

User story

As a wallet integrator (Rust embedder, FFI consumer, or desktop-app author), I want a ready-made storage crate for platform-wallet so that I can persist wallet state across restarts without writing my own schema, my own migration pipeline, or my own backup/restore tooling — and I want the persister to take an automatic backup before anything destructive so I cannot accidentally lose data on a bad migration or a wrong wallet ID. Later, I want the same crate to offer a SecretStore abstraction so I do not have to roll my own OS-keyring integration on every platform.

What was done?

Two crates touched

  1. platform-wallet (packages/rs-platform-wallet/) — added an optional serde Cargo feature that gates #[derive(Serialize, Deserialize)] on every changeset type. Feature is off by default; library behaviour unchanged when not activated. Cherry-pickable into its own upstream PR via commit e26945cfdf.
  2. platform-wallet-storage (packages/rs-platform-wallet-storage/, new) — the actual storage crate. Module layout:
src/
├── lib.rs               # re-exports + (placeholder) `pub mod secrets;`
├── error.rs
├── sqlite/              # everything SQLite-specific lives here
│   ├── persister.rs     # impl PlatformWalletPersistence for SqlitePersister
│   ├── buffer.rs        # per-wallet ChangeSet merge buffer
│   ├── config.rs
│   ├── migrations.rs    # refinery + barrel
│   ├── backup.rs        # online backup, restore, retention
│   ├── schema/*.rs      # one writer/reader module per logical area
│   └── migrations/*.rs
├── secrets/             # RESERVED slot for the future SecretStore submodule (not implemented)
└── bin/
    └── platform-wallet-storage.rs   # clap-based CLI

Cargo features: default = ["sqlite", "cli"]. --no-default-features produces a bare crate so future secrets-only consumers can opt in just to that.

Highlights

  • Trait implementationSqlitePersister implements PlatformWalletPersistence (sync, Send + Sync, object-safe behind Arc<dyn …>).
  • Schema — 18 tables (per-wallet keyed) covering core state, identities, identity keys (public material only — verified by tests/secrets_scan.rs), contacts, platform addresses, asset locks, token balances, DashPay overlays, wallet metadata, account registrations, address pools. Single .db holds many wallets.
  • Foreign keys enabled with ON DELETE CASCADE. Composite-key parent-existence + cascade are trigger-emulated (barrel's sqlite3 backend cannot emit composite FK clauses portably; PRAGMA foreign_keys = ON only enforces FKs declared in CREATE TABLE).
  • Buffer-then-flush — single Mutex<HashMap<WalletId, PlatformWalletChangeSet>> merging via upstream Merge impls; one flush() call = one SQLite transaction. FlushMode::{Manual, Immediate} switch + commit_writes() end-of-batch hook.
  • Migrations via refinery 0.9 + barrel. Append-only.
  • Online backup / restore / retention via rusqlite::backup::Backup::run_to_completion. Retention by keep_last_n + max_age AND-semantics. TOCTOU-safe restore via tempfile::NamedTempFile::new_in(parent_dir) + atomic persist.
  • Automatic backups before destructive ops — pre-migration in open, pre-delete in delete_wallet. Library returns typed AutoBackupDisabled if auto_backup_dir = None; CLI escape hatch is a separate delete_wallet_skip_backup sibling method (does NOT mutate config — keeps library safe-by-default).
  • CLI bin platform-wallet-storage with backup, restore, prune, inspect, migrate, delete-wallet subcommands. Destructive ops require --yes. -v/-q wired to tracing_subscriber. Unix stream conventions.
  • Encoderbincode::serde::encode_to_vec over serde-derived changeset types No hand-rolled binary format. One carve-out: dpp's IdentityPublicKey uses #[serde(tag = "$formatVersion")] which bincode-serde rejects, so identity_keys.rs uses a wire-shape struct that re-encodes that one field via dpp's native bincode 2 derives — one blob per row preserved.
  • Secrets policy documented in SECRETS.md. The reserved src/secrets/ slot is the only directory exempt from the secrets_scan.rs forbidden-substring grep.
  • Workspace integration — added to Cargo.toml members, Dockerfile COPY --parents blocks (3 stages), .github/workflows/tests-rs-workspace.yml (--package lists in both shards), .github/workflows/pr.yml allowed-scope (wallet-storage).

Commits (strict file boundaries for clean cherry-pick)

SHA Title Scope
0f9437cd44 feat(wallet-sqlite): add platform-wallet-sqlite crate initial crate scaffolding
adf421257c ci(wallet-sqlite): wire crate into workspace CI, Dockerfile, and Cargo.toml workspace integration
cea9ddad4d fix(wallet-sqlite): library/CLI/tests/docs fix wave from Phase 3 QA post-QA fixes
e26945cfdf feat(platform-wallet): add optional serde derives behind serde feature only packages/rs-platform-wallet/ — independently cherry-pickable
8e0830626d refactor(wallet-storage): rename platform-wallet-sqlite → platform-wallet-storage and restructure for future secrets submodule pure rename + reshape
74acc8152b refactor(wallet-storage): use bincode-serde for BLOB columns, remove hand-rolled encoder -313 net LOC, +6 previously-deferred tests unblocked
5bac6e304d refactor(wallet-storage): drop per-blob schema-rev tag, rely on migration version for forward-compat -33 net LOC, blob.rs trimmed to thin bincode-serde wrappers
540decf652 refactor(wallet-storage): drop --dry-run from prune CLI -53 net LOC, helper + flag + docs
4cfec30375 fix(platform-wallet): correct stale crate name in doc comment after wallet-storage rename upstream PR cherry-pick alongside e26945c
bd4216dbe2 refactor(wallet-storage): rename SqlitePersisterError → WalletStorageError, atomic variants, propagate SQL errors error rework + safe_cast + LoadIncomplete + pre-restore backup + chmod 600 + integrity recheck
f58e784593 fix(wallet-storage): SEC-003 defensive update triggers + build-script migration tracking V002 BEFORE-UPDATE rejection triggers on wallet_id / identity_id
87f38c0f15 chore(wallet-storage): post-review cleanup (delete CHANGELOG, JSON escaping, scope allow-list, stable enum labels, docs) serde_json, db_label helpers, deletes CHANGELOG.md, drops wallet-sqlite scope
f45ab091ae fix(platform-wallet-storage): drain buffer on wallet delete (CMT-001) drain-and-discard buffered changeset first; existence gate widened to buffered-or-persisted
f18a1e4b8c fix(platform-wallet-storage): recheck schema/version on staged copy (CMT-002) schema-history + max-version checks moved onto the staged temp copy; closes restore TOCTOU
bed413e513 fix(platform-wallet-storage): reject trailing bytes in blob decode (CMT-003) blob::decode enforces bincode bytes-consumed == input length
e2746c3207 test(platform-wallet-storage): make tc054 root-safe (CMT-004) regular-file-as-parent yields ENOTDIR for every UID incl. root

Workflow

Built end-to-end via the claudius:workflow-feature skill: Diziet's Requirements + DX spec (26 FRs, 10 NFRs, 15-variant error catalogue) → Marvin's 83-case test specification → Bilby's implementation → parallel Phase-3 QA wave (Marvin / Smythe / Adams / Diziet / Trillian) → Bilby's fix wave (resolved 2 CRITICAL from Adams, 1 HIGH from Diziet, 2 HIGH from Marvin, 3 HIGH from Adams, 4 MEDIUM from Smythe / Trillian / Diziet) → reshape (crate rename + serde swap) → Lessons Learned (18 memories persisted).

How Has This Been Tested?

  • `cargo test -p platform-wallet-storage` — full suite green, 0 failures, 0 ignored (1 proptest oracle in slow-running mode). Includes the regression tests added for the CodeRabbit fixes (tests/sqlite_delete_buffer_reconcile.rs, tests/sqlite_restore_staged_validation.rs, and the blob::decode trailing-bytes negative test).
  • `cargo build -p platform-wallet-storage` (with and without --no-default-features) — clean.
  • `cargo build -p platform-wallet-storage --bin platform-wallet-storage` — clean.
  • `cargo build -p platform-wallet --features serde` — clean (opt-in feature off by default; both states verified).
  • `cargo clippy -p platform-wallet-storage -p platform-wallet --all-targets -- -D warnings` — clean.
  • `cargo fmt --all -- --check` — clean.
  • `cargo check --workspace --offline` — clean (no regressions elsewhere).

Coverage gained in the encoder swap (previously deferred per-sub-changeset round-trips, now green): TC-007 (identity keys), TC-009 (platform addresses), TC-010 (asset locks incl. embedded Transaction), TC-012 (DashPay overlays), TC-014 (account registration with full ExtendedPubKey).

Deferred (with rationale, follow-up PRs)

  • ClientStartState.wallets reconstruction in load() — blocked on a future upstream Wallet::from_persisted. All data IS persisted; only the wallet rehydration step is gated. TODO in persister.rs::load.
  • A handful of fault-injection-seam test cases (TC-021 / TC-024) — partial-rollback under injected failure; queued behind the existing lock_conn_for_test test-helper accessor.
  • TC-003, TC-006 (InstantLock, Transaction direct round-trips) — kept on consensus::Encodable for those bytes (canonical Bitcoin-style wire format); a future PR may unify on serde if dashcore gains compatible derives.
  • Workspace-wide bincode 2.0.1 advisory (RUSTSEC-2025-0141, unmaintained) — workspace concern, not crate-specific.
  • Upstream key_wallet::Wallet's WalletType::Mnemonic does not zeroize its seed fields on drop. Out of scope; flagged for upstream follow-up.

Breaking Changes

None. New crate, opt-in. Existing NoPlatformPersistence remains in place and is unchanged. The new platform-wallet/serde feature is off by default.

Checklist

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have added or updated relevant unit/integration/functional/e2e tests
  • I have added "!" to the title and described breaking changes in the corresponding section if my code contains any (N/A — no breaking changes)
  • I have made corresponding changes to the documentation if needed (README, SECRETS.md, rustdoc on every public item)

For repository code-owners and collaborators only

  • I have assigned this pull request to a milestone

🤖 Co-authored by Claudius the Magnificent AI Agent

Summary by CodeRabbit

  • New Features

    • SQLite wallet storage with migrations, integrity checks, online backup/restore, auto-backup retention/pruning, configurable buffering/flush modes, and an exported maintenance CLI
    • Optional serde feature for serializing changesets and related types
  • Documentation

    • Detailed README and secrets guidance for the wallet storage component
  • Tests

    • Extensive integration/unit tests covering persistence, backup/restore, retention, integrity, error classification, and CLI smoke tests
  • Chores

    • CI/workspace, Docker build, and PR-title linter updated to include the new storage package and scope

Review Change Stack

lklimek and others added 3 commits May 11, 2026 12:24
New workspace crate `platform-wallet-sqlite` implementing the
`PlatformWalletPersistence` trait against a bundled SQLite backend, plus
a `platform-wallet-sqlite` maintenance CLI.

Highlights
- Per-wallet in-memory buffer with `Merge`-respecting `store` + atomic
  per-wallet `flush` (one SQLite transaction per call).
- `FlushMode::{Immediate, Manual}` with `commit_writes` aggregating
  dirty wallets in deterministic order.
- Online backup via `rusqlite::backup::Backup::run_to_completion`,
  source-validating `restore_from`, `prune_backups` retention with
  AND-semantics, automatic pre-migration and pre-delete backups (with
  typed `AutoBackupDisabled` refusal when `auto_backup_dir = None`).
- Refinery-driven barrel migrations under `migrations/`; FK enforcement
  emulated with triggers because barrel's column builder doesn't emit
  composite-key `FK` clauses portably on SQLite.
- `delete_wallet` cascade with `DeleteWalletReport`; `inspect_counts`
  surface for the CLI.
- CLI: `migrate`, `backup`, `restore`, `prune`, `inspect`,
  `delete-wallet` with `--yes` destructive-op guards, humantime
  retention parsing, and stdout/stderr/exit-code conventions matching
  the spec.
- 52 tests across 8 files plus compile-time assertions cover every
  FR/NFR except the ones blocked on upstream `serde`/`bincode`
  derives or a `Wallet::from_persisted` constructor (tracked in
  TODOs in `persister.rs::load` and the test modules' module-docs).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…o.toml

Phase 2.2 fix wave — addresses Adams' BLOCK findings.

- PROJ-001: add `platform-wallet-sqlite` to both `--package` lists in
  `tests-rs-workspace.yml` (coverage run and the Ubuntu 4-shard
  fallback) so CI actually executes the crate's tests.
- PROJ-002: append `packages/rs-platform-wallet-sqlite` to every
  enumerated `COPY --parents` block in the Dockerfile (the chef
  prepare stage, the artifact-build stage, and the rs-dapi stage).
  Workspace `Cargo.toml` already lists the member; chef would fail
  with "directory not found" without these copies.
- PROJ-003: allow `wallet-sqlite` in the PR-title conventional-
  scopes list (matches the existing `feat(wallet-sqlite): …` commit).
- PROJ-004: align `dash-sdk` feature flags with sibling
  `rs-platform-wallet` (`dashpay-contract`, `dpns-contract`); document
  why `dpp`, `dash-sdk`, and `bincode` are direct deps (they're
  actually used — Adams' "unused" claim was wrong for all three);
  drop the redundant `serde` feature from bincode.
- PROJ-005: gate `lock_conn_for_test` and `config_for_test` behind
  `cfg(any(test, feature = "test-helpers"))` plus a new
  `test-helpers` dev feature; the crate's own `[dev-dependencies]`
  self-include now activates it for integration tests, so downstream
  consumers cannot reach the helpers.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 2.2 fix wave — addresses Diziet, Marvin, Smythe, Trillian BLOCKs.

Library
- D-01: new `SqlitePersister::delete_wallet_skip_backup(wallet_id)`
  entry point that intentionally skips the auto-backup. The CLI's
  `--no-auto-backup` now uses it instead of mutating
  `auto_backup_dir` to `None` (which collided with the
  `AutoBackupDisabled` refusal path and silently broke the flag).
- D-02: `delete_wallet` checks `wallet_metadata` existence BEFORE
  running the auto-backup. Refusing on an unknown wallet id no
  longer leaves an orphaned `.db` in the auto-backup directory.
- D-03: `restore_from` try-acquires an exclusive file lock on the
  destination via `fs2::FileExt::try_lock_exclusive` and raises
  `RestoreDestinationLocked` if the file is held. Falls through on
  filesystems without advisory locking.
- D-04: `restore_from` reads the source DB's max
  `refinery_schema_history.version` and raises
  `SchemaVersionUnsupported { found, expected_range }` when it
  exceeds the highest embedded migration version.
- SEC-001: `restore_from` stages via
  `tempfile::NamedTempFile::new_in(parent)` plus `persist`. The
  previous predictable `<dest>.db.restore-tmp` filename was a
  symlink-plant TOCTOU window.
- DOC-007 / DOC-008: rustdoc on `RetentionPolicy` explains the
  AND-semantics; `DeleteWalletReport.backup_path` documents that
  `None` ONLY happens via the new skip-backup entry point.

CLI
- D-05: `-v`/`-vv`/`-vvv`/`-q` wired to a `tracing_subscriber::fmt`
  subscriber that writes to stderr with an `EnvFilter` defaulted
  from the flag count (`warn` / `info` / `debug` / `trace`); `-q`
  forces `error`.
- `delete-wallet --no-auto-backup` now routes through
  `delete_wallet_skip_backup` and prints empty stdout (no backup
  path) with the `warning: auto-backup skipped (--no-auto-backup)`
  line on stderr.

Tests
- QA-001: new TC-023 in `tests/buffer_semantics.rs` — registers a
  `commit_hook` on the write connection (rusqlite `hooks` feature),
  then drives a flush whose changeset touches `core_sync_state`,
  `wallet_metadata`, and `token_balances`. The hook MUST fire
  exactly once. Atomicity is now empirically verified.
- QA-008: `tests/load_reconstruction.rs::tc043_*` rewritten to
  store non-empty `ContactChangeSet` and `TokenBalanceChangeSet`
  payloads (the previous Defaults were `is_empty()` and got
  skipped by the buffer). The test now reopens the persister,
  directly SQL-queries `contacts_sent` and `token_balances` rows,
  and asserts `ClientStartState.platform_addresses` stays empty.
- SEC-006: new `tests/secrets_scan.rs` greps every file under
  `src/schema/` and `migrations/` for the substrings `private`,
  `mnemonic`, `seed`, `xpriv`, `secret`. A small allow-list lets
  doc comments mention the boundary while catching genuine slips.

Docs
- DOC-002: README CLI synopsis adds an explicit sentence about
  `--yes` being REQUIRED for destructive subcommands, plus a
  logging-flag blurb.
- DOC-016: new per-crate `CHANGELOG.md` with `[Unreleased]` section
  enumerating the additions and security fixes from this fix wave
  (the workspace CHANGELOG is generated from Conventional Commits).
- SECRETS.md audit-hooks section updated to point at
  `tests/secrets_scan.rs` and the TC-082 lint test by file:line.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@github-actions github-actions Bot added this to the v3.1.0 milestone May 11, 2026
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 11, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

<review_stack_artifact_start>
<stack_title>Add SQLite wallet storage crate</stack_title>
<stack_summary>Introduces a new Rust crate providing SQLite-backed platform wallet persistence with migrations, buffering, backup/restore, schema writers/readers, CLI, utilities, tests, and workspace/CI/Docker and serde wiring.</stack_summary>

New crate implementing SqlitePersister (buffered writes, flush semantics), embedded migrations, online backups/restores and pruning, per-area schema writers/readers, utilities (permissions, safe_cast), a maintenance CLI, extensive tests, and workspace/CI/Docker/serde integration. All added crate metadata, build/migrations, persister, backup/restore, schema modules, utilities, CLI, tests, workspace/CI/Docker updates, and platform-wallet serde feature wiring grouped for end-to-end review. range_264ca1d03417 range_4a569c4bcec3 range_9b3021d37e60 range_104ae69d9ce2 range_56d88d90db4d range_df86bfcff835 range_a1a35732cca2 range_b2bbf315a66a range_0c3777524fa4 range_565c09732f63 range_2ae09a4d16d4 range_f1252c57e271 range_eefe65f93e20 range_28648506588c range_4aa872434199 range_e2517d1d8b70 range_6eda9b5f2fa2 range_502d0b3966f1 range_f5f6715a4e04 range_43f151759d5a range_ba1234c721d4 range_f6c378dadceb range_a7b65d2c01ee range_b5db0b187170 range_8825497ba4c1 range_380de8151c19 range_9edd341ed9a7 range_b64f8b400c8f range_6d109913f584 range_0886cfca33c7 range_fd8ad33edeac range_1f989a8a72bd range_db03220ccc2c range_696c459cd216 range_d2bda1bd1f13 range_f3a636bcb903 range_bd90d492f75f range_1f583d8fe876 range_3ca15bf119e3 range_4efd503e7b47 range_e8709f17b9dc range_0476f08821a1 range_00c04804366e range_bd3fe6a54d4c range_7b6345b17c61 range_41c2bb429249 range_b5eb8ee55155 range_8e83bde7eb7e range_f763c2abf55d range_a4cb110b37b9 range_ec96a6dcca2b range_4bc8857ab7d1 range_181ad365885a range_c4175350bf29 range_ca9fad4e4c8f range_761db3b2a472 range_da16271a79fd range_e5519d33f878 range_3823435cb011 range_79f85b7a4626 range_18fc49987a30 range_4929ed6fde93 range_b054e7ac12d6 range_1bdb980b1bc8 range_400922ff143b range_ea5b4859c454 range_9092d5aee97d range_fe7bb48db442 range_bca7659268fe range_0363e0e4a938 range_bc02fc4c7578 range_f8b403e3bede range_e61148c6db57 range_0db3451eb6eb range_c2946f9cd36d range_bfec27c4b540 range_8a250add163a range_af9a8520f8c3 range_5e69d5cbb8fa range_6052fde80c2e range_b97a0bec751c range_eeea185baca7 range_500cc102002b range_2bb92a775270 range_7e54e6c1f3a4 range_5822e6efdd19 range_b1fe5efaa8f4 range_e4478389012d range_04e8827bf5e3 range_7b2b5ad7eea4 range_d32660f67c0b range_467e7675b8fb range_f747b4132610 range_6e347f888a5e range_e3c26c5fd329 range_dae44726b645 range_030a63f2fb14 range_e61bf117f946 range_60d4c4cd75b8 range_8c684dcd9158 range_efc017b64c27 range_c2a0a8681b47 range_229a017ff5a6 range_95d8de0987e0 range_7f520d8b05b6 range_23508a78cde2 range_b6b41d2c4f18 range_8e76505e8011 range_80424be4509f range_730a3812a5d2 range_d750070c02f4 range_1835a2f250be range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb range_3c253b6b31de range_63b1de62b1b4 range_001f73c931bb
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/platform-wallet-sqlite-persistor

@Claudius-Maginificent Claudius-Maginificent changed the title feat(wallet-sqlite): add platform-wallet-sqlite crate feat(platform-wallet): add platform-wallet-sqlite persister crate May 11, 2026
lklimek and others added 3 commits May 11, 2026 14:20
Add a new `serde` Cargo feature on `platform-wallet`. When enabled,
every type carried in a `PlatformWalletChangeSet` gains
`serde::Serialize` / `serde::Deserialize` derives via
`#[cfg_attr(feature = "serde", derive(...))]`:

- `CoreChangeSet`, `IdentityChangeSet`, `IdentityEntry`,
  `IdentityKeysChangeSet`, `IdentityKeyEntry`,
  `IdentityKeyDerivationIndices`, `ContactChangeSet`,
  `ContactRequestEntry`, `SentContactRequestKey`,
  `ReceivedContactRequestKey`, `PlatformAddressChangeSet`,
  `PlatformAddressBalanceEntry`, `AssetLockChangeSet`,
  `AssetLockEntry`, `TokenBalanceChangeSet`,
  `WalletMetadataEntry`, `AccountRegistrationEntry`,
  `AccountAddressPoolEntry`, and the top-level
  `PlatformWalletChangeSet`.
- Per-identity / DashPay leaf types referenced inside those
  changesets: `BlockTime`, `IdentityStatus`, `DpnsNameInfo`,
  `DashPayProfile`, `ContactRequest`, `EstablishedContact`,
  `PaymentEntry`, `PaymentDirection`, `PaymentStatus`,
  `AssetLockStatus`.

The feature activates `key-wallet/serde` (which transitively flips
`dashcore/serde` and `dash-network/serde`) so every upstream leaf
type already wired with `#[cfg_attr(feature = "serde", ...)]`
(TransactionRecord, Utxo, InstantLock, AccountType, AddressInfo,
AddressPoolType, ExtendedPubKey, Network) round-trips cleanly.

Two upstream types lack their own serde feature and use
`#[serde(with = ...)]` adapters in the new
`src/changeset/serde_adapters.rs` module:
- `AssetLockFundingType` (key-wallet, no `serde` derive) — encoded
  as a stable u8 tag matching the prior hand-rolled blob layout.
- `AddressFunds` (dash-sdk re-export, no serde derive) — encoded
  as a `(nonce, balance)` shadow struct.

One field is marked `#[serde(skip)]`:
- `CoreChangeSet::addresses_derived` carries
  `key_wallet_manager::DerivedAddress`, which has no serde derive
  AND no `key-wallet-manager/serde` feature to activate. The
  breadcrumb is written to a typed table by persisters, not via a
  changeset blob, so skipping costs nothing.

`cargo build -p platform-wallet` (no features) and
`cargo build -p platform-wallet --features serde` both build
clean. `cargo test -p platform-wallet` passes (8 lib tests, 121
integration tests) with and without the new feature. The change
is opt-in; the default-feature build is byte-identical to its
prior shape.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…allet-storage and restructure for future secrets submodule

PURE rename + restructure — no functional code changes. Carves out a
spot for a future `SecretStore` (sketched in `SECRETS.md`) to land
as a `secrets` submodule inside the same crate, rather than a
separate `platform-wallet-secrets` crate.

Crate metadata
- Cargo package name: `platform-wallet-sqlite` → `platform-wallet-storage`.
- Crate directory: `packages/rs-platform-wallet-sqlite/` →
  `packages/rs-platform-wallet-storage/`.
- Binary name: `platform-wallet-sqlite` → `platform-wallet-storage`.

Module layout
- Everything SQLite-related is now under `src/sqlite/`:
  `mod.rs` (new — re-exports the submodules), `persister.rs`,
  `buffer.rs`, `config.rs`, `error.rs`, `migrations.rs`, `backup.rs`,
  and `schema/`. The `migrations/` Rust-file directory stays at the
  crate root because `refinery::embed_migrations!` resolves its path
  relative to `Cargo.toml`.
- `src/lib.rs` exposes `pub mod sqlite;` plus root re-exports of the
  common types (`SqlitePersister`, `SqlitePersisterConfig`,
  `FlushMode`, `SqlitePersisterError`, `RetentionPolicy`,
  `PruneReport`, `DeleteWalletReport`, `AutoBackupOperation`,
  `JournalMode`, `Synchronous`) so most consumer imports stay
  identical — only the crate name in `Cargo.toml` changes for them.
  A `// pub mod secrets;` marker reserves the future module slot.

Cargo features
- `sqlite` (default) — enables the SQLite persister + every backend-
  specific optional dep (`rusqlite`, `refinery`, `barrel`, `dpp`,
  `dash-sdk`, `key-wallet`, `key-wallet-manager`, `dashcore`,
  `bincode`, `fs2`, `tempfile`, `chrono`, `sha2`).
- `cli` (default) — enables the maintenance binary; implies `sqlite`.
- `secrets` — reserved, no code yet.
- `test-helpers` — crate-private accessors (unchanged semantics);
  now implies `sqlite`.
- `cargo build -p platform-wallet-storage --no-default-features`
  builds the bare crate cleanly (verified).

Tests
- Renamed `tests/<name>.rs` → `tests/sqlite_<name>.rs` (9 files) so
  the future `secrets_<name>.rs` files won't collide. `secrets_scan.rs`
  and `tests/common/` keep their names.
- `secrets_scan.rs` updated to scan `src/sqlite/schema/` (the new
  location of the schema writers) and `migrations/`. Carved out
  `src/secrets/` from the scan up front — that future submodule WILL
  legitimately contain the words `private`, `mnemonic`, `seed`.

Workspace integration
- `Cargo.toml` workspace `members` entry renamed.
- `Dockerfile`: three `COPY --parents` blocks updated.
- `.github/workflows/tests-rs-workspace.yml`: two `--package` lines
  updated.
- `.github/workflows/pr.yml`: added `wallet-storage` alongside the
  existing `wallet-sqlite` allow-list entry (both coexist so PRs
  pending against either name pass).

Gate output
- `cargo fmt --all -- --check` clean.
- `cargo build -p platform-wallet-storage` clean.
- `cargo build -p platform-wallet-storage --no-default-features` clean.
- `cargo build -p platform-wallet-storage --bin platform-wallet-storage` clean.
- `cargo test -p platform-wallet-storage` — 54 tests, 0 failures.
- `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean.
- `cargo check --workspace --offline` clean.
- `cargo metadata` no longer exposes the old `platform-wallet-sqlite`
  package name.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…hand-rolled encoder

Replace the hand-rolled `BlobWriter` / `BlobReader` plumbing under
`src/sqlite/schema/` with a single `bincode::serde::encode_to_vec`
call per row, acting on the serde-derived changeset types in
`platform-wallet` (enabled via that crate's `serde` feature, added in
the preceding commit). The encoder swap is the technical-debt cleanup
the workflow-feature plan called for.

Wire format
- Every `_blob` column now starts with a 1-byte schema-revision tag
  (`blob::BLOB_REV = 1`) followed by the bincode-serde body. The tag
  lets future migrations swap encoders without losing existing rows;
  unknown revisions surface as `SqlitePersisterError::Serialization`.
- `blob::encode<T: Serialize>` and `blob::decode<T: DeserializeOwned>`
  are the only public entry points; the previous per-field
  `u8/u32/u64/bytes/opt_*/str` walker is gone.
- The outpoint helpers (`encode_outpoint` / `decode_outpoint`) stay
  in `blob.rs` because outpoints serve as primary-key fragments —
  they were never `_blob` payloads to begin with.

Per-schema-file delta
- `accounts.rs`: dropped the manual `BlobWriter` for both
  `AccountRegistrationEntry` and `AccountAddressPoolEntry`; each row
  now encodes the full entry via `blob::encode`. Schema-stable typed
  columns (`account_type`, `account_index`, `pool_type`) still mirror
  the entry for direct SQL lookups.
- `asset_locks.rs`: collapsed the funding-type-tag / tx-consensus /
  proof-bincode three-part hand-rolled blob into a single
  `blob::encode(&AssetLockEntry)` call. `funding_type` rides through
  the new `platform_wallet::changeset::serde_adapters::asset_lock_funding_type`
  adapter; `Transaction` and `AssetLockProof` round-trip via their
  own serde derives. ~30 LOC removed.
- `contacts.rs`: each `_blob` cell now stores the
  `ContactRequestEntry` / `EstablishedContact` directly.
- `core_state.rs`: `core_transactions.record_blob` now encodes the
  full `TransactionRecord`; `core_instant_locks.islock_blob` encodes
  the `InstantLock` via dashcore's serde derive (which was always
  there, gated on `dashcore/serde` — flipped on by `platform-wallet/
  serde`). The placeholder-record decoder gymnastics in
  `get_tx_record` collapse into a one-line `blob::decode` call.
- `dashpay.rs`: `dashpay_profiles.profile_blob` encodes the whole
  `DashPayProfile`; `dashpay_payments_overlay.overlay_blob` encodes
  each `PaymentEntry`.
- `identities.rs`: `entry_blob` encodes the full `IdentityEntry`;
  new `fetch` helper for tests.
- `identity_keys.rs`: dpp's `IdentityPublicKey` uses
  `serde(tag = "$formatVersion")` which bincode-serde's
  `deserialize_any` requirement can't navigate. Solution: an
  in-crate wire shape (`IdentityKeyWire`) pre-encodes that one field
  via dpp's native `bincode::Encode/Decode` derives while everything
  else stays on bincode-serde. Same "one blob per row" property; one
  layer of indirection for the offending field.

Unblocked tests (Marvin's previously-deferred TC-002..TC-014)
- TC-007 — `IdentityKeyEntry` round-trip including the public key,
  hash, and DIP-9 derivation breadcrumbs; plus an inline NFR-10
  substring scan that asserts the blob contains no
  `private`/`mnemonic`/`seed`/`xpriv` ASCII.
- TC-009 — `PlatformAddressBalanceEntry` round-trip including the
  `AddressFunds` (via the `address_funds` serde adapter).
- TC-010 — `AssetLockEntry` round-trip including the embedded
  `Transaction`, `AssetLockFundingType` (via the
  `asset_lock_funding_type` adapter), and `AssetLockStatus`.
- TC-012 — `DashPayProfile` + `PaymentEntry` round-trip through the
  dashpay tables.
- TC-014 — `AccountRegistrationEntry` round-trip including the
  full `ExtendedPubKey` (via key-wallet's serde derive).

Gate output
- `cargo fmt --all -- --check` clean.
- `cargo build -p platform-wallet-storage` clean.
- `cargo build -p platform-wallet-storage --no-default-features` clean.
- `cargo build -p platform-wallet-storage --bin platform-wallet-storage` clean.
- `cargo test -p platform-wallet-storage` — 60 tests, 0 failures (up
  from 54 before this commit; +5 new TCs in
  `sqlite_persist_roundtrip.rs` plus +1 in the blob.rs lib-test
  suite).
- `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean.
- `cargo check --workspace --offline` clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Claudius-Maginificent Claudius-Maginificent changed the title feat(platform-wallet): add platform-wallet-sqlite persister crate feat(platform-wallet): add platform-wallet-storage crate (sqlite persister) May 11, 2026
lklimek and others added 7 commits May 11, 2026 15:19
…tion version for forward-compat

The refinery migration version on the database already gates schema
evolution at the right granularity — every row in every `_blob`
column is written by code at the same revision, so a per-blob
revision byte was redundant.

Changes
- `src/sqlite/schema/blob.rs`: remove the `BLOB_REV` constant and
  its prepend / strip logic. `encode<T>` is now a one-line wrapper
  over `bincode::serde::encode_to_vec`; `decode<T>` is the matching
  pair over `decode_from_slice`. Net: ~30 LOC dropped from the
  module.
- Drop the two unit tests (`decode_rejects_unknown_rev`,
  `decode_rejects_empty_blob`) that exercised the rev-tag logic
  exclusively — the behaviour they covered no longer exists. The
  `encode_decode_roundtrip` and `outpoint_roundtrip` tests stay.
- `src/sqlite/schema/mod.rs`: update the module-level encoding-policy
  doc to drop the "1-byte schema-rev tag" framing and explain that
  schema evolution is gated by the refinery migration version
  instead.
- `src/sqlite/schema/asset_locks.rs`: drop the analogous comment
  about the rev tag in that module's header.

`encode_outpoint` / `decode_outpoint` are untouched — they're a
separate concern (typed-column primary-key encoding, fixed layout
for indexed lookups, never blob payloads).

Migration concern: NONE. The crate is unreleased; no existing on-disk
`.db` files carry the BLOB_REV byte. Anyone with a wallet-storage
test database between the previous commit and this one needs to
delete it — flagged in the workspace CHANGELOG.

Gate
- `cargo fmt --all -- --check` clean.
- `cargo build -p platform-wallet-storage` clean.
- `cargo build -p platform-wallet-storage --bin platform-wallet-storage` clean.
- `cargo test -p platform-wallet-storage` — 58 tests, 0 failures
  (down from 60: the two dropped tests were rev-tag-specific).
- `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The `prune` subcommand returns to the unconditional shape: walk the
backup directory, apply the retention policy, unlink, print removed
paths to stdout. Operators who want a preview can list the directory
themselves before running.

Changes
- `src/bin/platform-wallet-storage.rs`: drop the `dry_run: bool`
  field on `PruneArgs`, the `if args.dry_run { ... }` branch in
  `run_prune`, and the `list_backup_dir_for_dry_run` helper (only
  caller was the dry-run branch).
- `README.md`: trim `[--dry-run]` from the `prune` synopsis line.
- `CHANGELOG.md`: note the flag removal in `[Unreleased]`.

No CLI smoke test referenced `--dry-run`, so the 58-test count is
unchanged. Gate is clean: fmt / build / bin build / 58 tests / clippy.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…allet-storage rename

PROJ-002: `CoreChangeSet.addresses_derived` doc block referenced
`rs-platform-wallet-sqlite::schema::core_state`, the path the crate
had before `8e0830626d` renamed it to `rs-platform-wallet-storage`
and regrouped the module layout under `sqlite/`. The rename swept
every import + Cargo.toml + workflow file but missed this single
doc-string in the sister crate, which a grep-driven reader would
follow to a dead path.

Replace with the current canonical path:
`platform_wallet_storage::sqlite::schema::core_state`.

No code change. No test change. Independently cherry-pickable into
the future upstream PR alongside `e26945cfdf` (the original
serde-feature commit).

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…Error, atomic variants, propagate SQL errors

Atomic-variant error type per the dash-evo-tool error pattern
(`~/git/dash-evo-tool/CLAUDE.md` §Error messages): every variant
carries the upstream error via `#[source]` (or `#[from]` when the
conversion is the only thing the trait does), never via a
stringified copy. Variants do not contain user-facing-prose
`String` fields — the `#[error("...")]` attribute provides the
renderable `Display` form, the typed fields carry diagnostics.

Resolves CODE-002, SEC-002, PROJ-001, CODE-004, CODE-008 (partial),
SEC-001 (library half — CLI half in Commit D). Annotates CODE-001
with INTENTIONAL per triage decision.

Error type
- `SqlitePersisterError` → `WalletStorageError`. The old name lives
  as a `#[deprecated]` type alias so existing callers compile during
  the migration; tests in this crate already use the new name.
- Split `Sqlite` callers into `IntegrityCheckRunFailed`,
  `SourceOpenFailed`, and the generic `Sqlite { source }`. The
  `IntegrityCheckFailed { check_output: String }` variant becomes
  `IntegrityCheckFailed { report: String }` — the SQLite-returned
  diagnostic text is not a user-facing message; the rename
  clarifies that.
- `Serialization(String)` (a stringified bincode error) split into
  `BincodeEncode { source: bincode::error::EncodeError }`,
  `BincodeDecode { source: bincode::error::DecodeError }`, and
  `BlobDecode { reason: &'static str }` for typed-column structural
  errors. `&'static str` is acceptable per the policy — it's a
  compile-time identifier, not a user message.
- `InvalidWalletId(String)` split into `InvalidWalletIdHex { source:
  hex::FromHexError }` and `InvalidWalletIdLength { actual: usize }`.
- `ConfigInvalid(&'static str)` → `ConfigInvalid { reason: &'static str }`.
- `SchemaVersionUnsupported { found: i64, expected_range: String }`
  → `SchemaVersionUnsupported { found: i64, max_supported: i64 }`.
- New variants: `HashDecode { source: dashcore::hashes::Error }`,
  `ConsensusCodec { source: dashcore::consensus::encode::Error }`,
  `IntegerOverflow { field: &'static str, value: u64, target:
  SafeCastTarget }`, `LoadIncomplete { unimplemented: &'static
  [&'static str] }`.
- `From` impls added for every typed source so `?`-style propagation
  works at every writer / reader boundary.
- `From<WalletStorageError> for PersistenceError` renders the full
  `#[source]` chain via a private `DisplayChain` helper instead of
  losing the inner-error context to a single `Display` call.

Safe-cast helper (SEC-002)
- New module `src/sqlite/util/safe_cast.rs` with `u64_to_i64(field:
  &'static str, value: u64) -> Result<i64, WalletStorageError>` and
  the inverse. Every durable-boundary cast in writers/readers now
  routes through these — schema/platform_addrs (balance, sync_height,
  sync_timestamp, last_known_recent_block, nonce, account_index,
  address_index), schema/asset_locks (amount_duffs, account_index),
  schema/token_balances (balance), schema/core_state (utxo.value,
  utxo.height, account_index), schema/identities (no u64 columns —
  identity_index is u32, uses `i64::from`).
- Lossless `u32 → i64` casts swapped to `i64::from(...)` so static
  conversions stay clearly distinct from fallible-cast sites.

Error propagation (CODE-002)
- Every `query_row(...).unwrap_or(default)` that previously
  swallowed real SQL errors (busy-timeout, corrupt, decode) now
  uses `.optional()?.unwrap_or(default)` — `optional()?` collapses
  ONLY the genuine "no rows returned" case into `None`; every other
  error propagates as `WalletStorageError::Sqlite`.
- `current_schema_version` and `count_pending` now return
  `Result<_, WalletStorageError>` instead of swallowing into
  `Option`. Migrate / open paths surface those errors instead of
  silently re-running every migration on a corrupt schema-history.
- `delete_wallet_inner` existence check + per-table row-count
  queries use `.optional()?` so a corrupt child table fails loudly
  instead of reporting 0 rows removed.

Auto-backup dedup (CODE-004)
- `run_auto_backup` extracted as a standalone function in
  `persister.rs`. Both the open-time (`PreMigration`) and library-
  time (`PreDelete`, new `PreRestore`) paths call it. The previous
  `unreachable!("OpenMigration not callable via run_auto_backup")`
  branch is gone — there is no longer a closed-over self that
  prevents the open path from reusing the helper.
- `BackupKind::PreRestore` variant added; `is_backup_file` /
  retention recognise the `pre-restore-` prefix.

LoadIncomplete (PROJ-001)
- `LOAD_UNIMPLEMENTED: &[&str]` pub-const lists the
  `ClientStartState` field paths the persister does not yet
  reconstruct (`["ClientStartState::wallets"]` today).
- Trait-impl `load()` rustdoc explicitly documents the partial-
  reconstruction caveat at the top, points at `LOAD_UNIMPLEMENTED`,
  and emits a `tracing::warn!` on every call until the upstream
  `Wallet::from_persisted` lands.
- New `WalletStorageError::LoadIncomplete` variant exists for
  callers that want to surface the gap as a typed value (not
  returned from `load` itself per the trait contract — see rustdoc).

restore_from auto-backup (SEC-001 library half)
- `SqlitePersister::restore_from(dest, src, auto_backup_dir)` —
  takes a pre-restore auto-backup of the live destination before
  staging the source over it. Refuses with
  `AutoBackupDisabled { operation: Restore }` when `auto_backup_dir`
  is `None`. New `SqlitePersister::restore_from_skip_backup(dest,
  src)` for the CLI's `--no-auto-backup` flag (added to RestoreArgs
  here for the corresponding CLI surface).
- `backup::restore_from` keeps the source-validation +
  destination-lock + staged-tempfile + atomic-persist shape; the
  pre-restore backup is taken by the persister's `_inner` before
  calling into `backup::restore_from`. (SEC-004 — staged-tempfile
  integrity recheck + chmod 600 — also lands in this commit.)

Write probe (CODE-008)
- `ensure_dir`'s predictable `.platform-wallet-storage-write-probe`
  filename replaced by `tempfile::NamedTempFile::new_in(dir)` —
  unguessable name per probe, no race against concurrent persister
  opens.

CODE-001 INTENTIONAL annotation
- Inline comment on the `Mutex<Connection>` declaration documents
  the accept-risk decision: single connection serializes reads
  through the write lock, acceptable for current per-wallet
  workload, revisit if read contention becomes measurable.

Test sweep
- Every `tests/sqlite_*.rs` file migrated from `SqlitePersisterError`
  to `WalletStorageError`. The deprecated alias still resolves but
  emits `#[deprecated]` warnings under `-D deprecated`; live code
  uses the new name. Restore tests call
  `SqlitePersister::restore_from_skip_backup` to avoid threading an
  `auto_backup_dir` through fixture helpers.

Gate
- `cargo fmt --all -- --check` clean.
- `cargo build -p platform-wallet-storage` clean (default features).
- `cargo build -p platform-wallet-storage --bin platform-wallet-storage` clean.
- `cargo test -p platform-wallet-storage` — 62 tests, 0 failures
  (+4 from new safe_cast unit tests).
- `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
… migration tracking

SEC-003: V001 emulates FK INSERT parent-existence + AFTER-DELETE
cascade via triggers but doesn't cover `UPDATE wallet_id` on
`wallet_metadata` or `UPDATE identity_id` on `identity_keys` /
`dashpay_profiles`. The persister's own writers never mutate those
columns, but if a future migration accidentally introduces such an
UPDATE the result is silent orphaning of child rows. New migration
`V002__defensive_update_triggers.rs` installs `BEFORE UPDATE OF
<id>` triggers on each that raise the canonical
`RAISE(ABORT, 'FOREIGN KEY constraint failed')` — same idiom V001
uses for the parent-existence check, so downstream string matching
stays stable.

V001 stays untouched per the append-only migration policy.

Also: `build.rs` emits `cargo:rerun-if-changed` for each file under
`migrations/`. `refinery::embed_migrations!` is a proc-macro
evaluated at compile time; Cargo doesn't track file-system reads
inside proc macros, so without this build-script directive,
adding/editing a migration file fails to trigger a rebuild of the
embedded list. Discovered while wiring V002 — `tc025` failed
against a stale cache until `migrations.rs` was manually touched.
The build-script closes that gap.

Gate
- `cargo fmt --all -- --check` clean.
- `cargo build -p platform-wallet-storage` clean.
- `cargo test -p platform-wallet-storage` — 62 tests, 0 failures.
- `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…caping, scope allow-list, stable enum labels, docs)

Closes the cleanup batch from the Phase-2.8 triage report:
PROJ-003, PROJ-004, SEC-005, SEC-006, CODE-003, DOC-002, DOC-005,
plus a related DOC-001 correction (FK README claim).

PROJ-003 — Remove `wallet-sqlite` from `.github/workflows/pr.yml`.
The three historical commits using that scope are already on the
branch; future commits in this crate use `wallet-storage`. No
reason to keep a deprecated name in the allow-list.

PROJ-004 — Delete `packages/rs-platform-wallet-storage/CHANGELOG.md`.
The user explicitly stated we don't maintain per-crate CHANGELOGs;
the workspace-level CHANGELOG.md is generated from Conventional
Commits and remains the single source of truth.

SEC-005 — Delete the substring-scan block in
`tests/sqlite_persist_roundtrip.rs::tc007_identity_key_entry_roundtrip`.
bincode wire bytes carry no field names, so the substring scan
against `public_key_blob` conveyed intent but enforced nothing.
The load-bearing NFR-10 check is `tests/secrets_scan.rs`, which
greps schema source files. Comment in tc007 redirects readers
there.

SEC-006 — Replace hand-rolled JSON in `run_inspect --format json`
with `serde_json::json!`. `serde_json` added as an optional dep
gated by the `cli` feature. Today's input is safe (table names are
compile-time identifiers; wallet ids are hex), but any future
addition that flows user-controlled bytes into the printer would
break the previous escape-less `print!`.

CODE-003 — `format!("{:?}", entry.account_type)` /
`format!("{:?}", entry.pool_type)` replaced with new pub(crate)
helpers `account_type_db_label(&AccountType) -> &'static str` and
`pool_type_db_label(&AddressPoolType) -> &'static str` in
`schema/accounts.rs`. Both are exhaustive `match` expressions —
adding a variant upstream fails to compile here, forcing an
explicit label decision rather than silent `Debug`-format drift.
`schema/core_state.rs` (derived-addresses writer) uses the same
helpers.

DOC-002 — `tests/secrets_scan.rs` docstring updated: scan path is
`src/sqlite/schema/` not `src/schema/`. Explicitly carves out files
in `src/sqlite/` outside `schema/` plus the future `src/secrets/`
slot as out-of-scope.

DOC-005 — README `--no-default-features` paragraph rewritten:
factual description of what the bare crate provides today (nothing
public), no future-feature framing per user's "no future
placeholders" rule.

DOC-001 (bonus correction) — README schema section updated to
reflect V002's defensive UPDATE triggers. The previous "identical
to native FKs" claim was false on UPDATE before V002; with V002
landed the claim becomes accurate and the section explicitly cites
both migrations.

INTENTIONAL annotations already in place from Commits B/C —
CODE-001 (single connection serialises reads) at
`src/sqlite/persister.rs:78-84`; CODE-007 (prune fails-fast) at
`src/sqlite/backup.rs:200-204`. PROJ-005's accept-risk rationale
is captured inline above the `lock_conn_for_test` accessor at
`src/sqlite/persister.rs:299-307`.

Gate
- `cargo fmt --all -- --check` clean.
- `cargo build -p platform-wallet-storage` clean.
- `cargo build -p platform-wallet-storage --no-default-features` clean.
- `cargo build -p platform-wallet-storage --bin platform-wallet-storage` clean.
- `cargo test -p platform-wallet-storage` — 62 tests, 0 failures.
- `cargo clippy -p platform-wallet-storage --all-targets -- -D warnings` clean.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@lklimek
Copy link
Copy Markdown
Contributor

lklimek commented May 11, 2026

@coderabbitai review all

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 11, 2026

@lklimek Sure! I'll kick off a full review of all the changes in this PR right away.

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 15

🧹 Nitpick comments (11)
packages/rs-platform-wallet-storage/src/sqlite/config.rs (1)

111-118: 💤 Low value

Minor: Simplify empty parent check.

The .filter(|p| !p.as_os_str().is_empty()) check at line 114 is defensive, but Path::parent() should not return an empty OsStr in practice for well-formed paths. The fallback to "." is safe regardless.

Consider simplifying to:

let parent = db_path.parent().unwrap_or_else(|| Path::new("."));
parent.join("backups").join("auto")

However, the current implementation is safe and may handle edge cases in path handling, so this is optional.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/config.rs` around lines 111 -
118, The function default_auto_backup_dir contains an overly defensive check
when computing parent from db_path; simplify by using
db_path.parent().unwrap_or_else(|| Path::new(".")) to get a &Path fallback and
then join "backups"/"auto" off that parent. Update the local variable used
(currently named parent) to hold the &Path from parent() rather than mapping to
a PathBuf, and then call parent.join("backups").join("auto") to produce the
final PathBuf.
packages/rs-platform-wallet-storage/tests/common/mod.rs (1)

47-56: 💤 Low value

Hardcoded 'testnet' may collide with tests asserting on network.

ensure_wallet_meta always writes network = 'testnet'. Tests that later assert on wallet metadata or persist a WalletMetadataEntry with a different network (e.g., tc023_one_flush_is_one_transaction writes Network::Testnet and would silently match, but other tests writing Mainnet would observe a stale row from this helper). Consider taking network as a parameter (defaulting to "testnet") so call sites can match their changeset's expectations.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/tests/common/mod.rs` around lines 47 -
56, The helper ensure_wallet_meta currently hardcodes network = 'testnet', which
can collide with tests expecting other networks; change the signature of
ensure_wallet_meta(persister: &SqlitePersister, wallet_id: &WalletId) to accept
a network parameter (e.g., network: &str) with callers passing "testnet" where
appropriate, update the INSERT SQL call in ensure_wallet_meta to bind the
network parameter instead of the literal 'testnet', and update all test call
sites (or add a convenience wrapper) to pass the correct network values (e.g.,
"mainnet" or "testnet") so tests see consistent wallet_metadata rows.
packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs (2)

188-231: 💤 Low value

Two if let Cmd::Migrate branches — fold them.

The Migrate command is special-cased twice in succession (lines 219–231 to tweak config / validate flags, then lines 235–244 to actually run it). Folding both into a single if let Cmd::Migrate(m) = &cli.cmd { ... return ... } block (or a run_migrate helper) would keep all of the migrate-specific logic in one place and remove the unreachable!() arm on line 247.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`
around lines 188 - 231, The migrate handling is split across two places; fold
the two separate `if let Cmd::Migrate` blocks into one contiguous block so all
migrate-specific validation and invocation live together. Locate the existing
`if let Cmd::Migrate(m) = &cli.cmd {` usage, move the checks that validate
`auto_backup_dir` and the `m.no_auto_backup` mutation of `config` (the use of
`SqlitePersisterConfig::new(&db)` and `config.with_auto_backup_dir(...)`) into
the same block where the migrate command is executed (or replace both with a
single `run_migrate` helper that accepts `db`, `m`, `config`, and
`auto_backup_dir`), then perform the `run_migrate` call and return its Result
immediately; finally remove the now-unnecessary second `if let Cmd::Migrate` and
the `unreachable!()` arm.

122-134: 💤 Low value

Reject uppercase / odd-length hex consistently.

parse_wallet_id checks s.len() == 64 then defers to hex::decode, which accepts both upper- and lower-case but not mixed-case-with-non-hex obviously — that's fine. However, the error message on line 124 leaks the raw user-supplied string back into stderr; if this binary is ever invoked from a context that pipes secrets-like opaque IDs, echoing them is undesirable. Consider dropping the trailing (`{}`) from the message and just naming the length, matching the wallet id is not valid hex style on line 130.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`
around lines 122 - 134, In parse_wallet_id, stop echoing the raw input in the
length error and validate/normalize hex consistently: when s.len() != 64 return
an Err that only mentions the expected length (do not include `s`), and replace
relying solely on hex::decode with an explicit check that all chars are
lowercase hex (0-9,a-f) so uppercase is rejected; keep the existing "wallet id
is not valid hex" error for decoding failures but ensure messages follow the
same non-leaking style.
packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs (2)

273-278: 💤 Low value

Remove redundant _unused_btreemap dead-code shim.

BTreeMap is now actually used in tc023_one_flush_is_one_transaction (see line 308), so the #[allow(dead_code)] fn _unused_btreemap workaround can be deleted along with its comment.

♻️ Suggested removal
-// Mark the unused `BTreeMap` import as used in case future expansion of
-// this test file needs it.
-#[allow(dead_code)]
-fn _unused_btreemap() -> BTreeMap<u32, u32> {
-    BTreeMap::new()
-}
-
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs` around
lines 273 - 278, Remove the redundant dead-code shim `_unused_btreemap` and its
explanatory comment: delete the `#[allow(dead_code)] fn _unused_btreemap() ->
BTreeMap<u32, u32> { BTreeMap::new() }` block since `BTreeMap` is now actually
used in the test `tc023_one_flush_is_one_transaction`, making the shim
unnecessary; ensure imports remain and run tests to confirm nothing else depends
on that helper.

154-180: 💤 Low value

Proptest opens a fresh SQLite DB per case — consider reusing one persister.

fresh_persister() runs once per proptest case (×64), each migrating a brand-new on-disk SQLite DB. That makes this test materially slower and more fragile on tmpfs/CI than necessary. Since the property is purely about monotonic-max merge on core_sync_state for a single wallet, you could hoist persister creation out of the proptest closure and just use a fresh wid per case (or reset the row), keeping the same DB across cases.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs` around
lines 154 - 180, Test currently calls fresh_persister() inside the proptest
closure causing a new on-disk SQLite DB per case; move persister creation out of
the proptest! Create the persister once before proptest (call fresh_persister()
once to get persister/_tmp/_path), then inside the proptest closure use a new
wallet id (wid) per case or clear/reset the core_sync_state row (use
ensure_wallet_meta and persister.store with core_with_height) so each case
reuses the same DB connection and only varies the wallet or row content; update
references to fresh_persister, wid, ensure_wallet_meta and persister
accordingly.
packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs (1)

72-102: 💤 Low value

ensure_exists writes outside the buffer/flush transaction boundary.

ensure_exists takes a &Connection and runs an immediate INSERT OR IGNORE rather than participating in the in-memory merge buffer + per-flush transaction model used by apply. This is fine for the documented use case (tests poking the DB before exercising identity_keys), but worth a doc note so production callers don't reach for it and accidentally couple stub writes to a different durability boundary than the rest of a changeset flush.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs` around
lines 72 - 102, ensure_exists currently performs an immediate INSERT OR IGNORE
on a plain &Connection, which bypasses the in-memory merge buffer and per-flush
transaction used by apply and can cause durability/ordering mismatches; update
ensure_exists to participate in the same flush boundary by either accepting and
using the existing merge buffer/flush transaction or a transactional handle
(e.g., a &Transaction or buffer API used by apply) and executing the INSERT
within that transaction, or if intentionally intended only for tests, add a
clear doc comment on ensure_exists describing that it writes immediately to the
DB and must not be used by production code that relies on the merge-buffer +
per-flush transaction model (reference ensure_exists, apply, and the in-memory
merge buffer/flush transaction behavior in your comment).
packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs (1)

20-26: ⚡ Quick win

Consider widening SafeCastTarget to cover u32-bound writes.

Several call sites (e.g., asset_locks::list_active, core_state::list_unspent_utxos) need to fail when an i64 won't fit in u32 and currently report SafeCastTarget::U64, which is wrong. Adding a U32 variant here (and a small i64_to_u32 helper) would let those readers go through this module with an accurate target label and consistent error shape.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs` around
lines 20 - 26, The SafeCastTarget enum currently only lists I64 and U64 but
callers need a U32 target; add a new variant SafeCastTarget::U32 and implement a
small helper function i64_to_u32 that mirrors existing i64_to_u64/i64_to_i64
behavior (validate range, return Result<u32, SafeCastError> and report
SafeCastTarget::U32 on failure). Update any match/log paths in this module that
construct errors to use the new U32 variant so callers like
asset_locks::list_active and core_state::list_unspent_utxos can call the helper
and produce the correct error shape/message.
packages/rs-platform-wallet-storage/migrations/V001__initial.rs (2)

185-191: 💤 Low value

Comment references inject_custom but the code appends raw DDL.

The comment claims constraints/indexes are layered "via inject_custom", but the implementation just concatenates DDL after m.make::<Sqlite>(). Either switch to barrel's inject_custom API or update the comment to reflect what's actually happening so future maintainers don't go looking for an inject_custom call site.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/migrations/V001__initial.rs` around lines
185 - 191, The comment mentions layering constraints/indexes "via
`inject_custom`" but the code builds raw DDL by concatenating into `tail` after
calling `m.make::<Sqlite>()`; either (A) change the implementation to use
Barrel's `inject_custom` API to append the constraints/indexes instead of manual
string concatenation (replace the `tail` concatenation and use
`m.inject_custom(...)` at the appropriate spot), or (B) update the comment to
accurately describe the current behavior (mention that raw DDL is appended to
`tail` after `m.make::<Sqlite>()` and that `inject_custom` is not used) so
future maintainers are not misled; locate references to `tail`,
`m.make::<Sqlite>()`, and any subsequent emission of the SQL to modify
accordingly.

281-326: 💤 Low value

Unused _cols parameter in parent_check closure.

The closure takes _cols: &[&str] but never uses it; every call passes &["wallet_id"]. Consider removing the parameter, or actually using it to parameterize the FK column name so the closure can support FK relationships keyed on something other than wallet_id (which would let you fold the bespoke identity_keys / dashpay_profiles triggers below into the same helper).

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/migrations/V001__initial.rs` around lines
281 - 326, The closure parent_check currently accepts an unused parameter _cols:
&[&str]; either remove that parameter from parent_check and all its callers
(keep it as parent_check(child: &str) and continue using wallet_id inside the
generated trigger SQL), or update parent_check to accept cols: &[&str] and use
cols[0] (or join cols) in place of the hard-coded wallet_id everywhere
(including the WHEN clauses and DELETE WHERE clause) so the same helper can
generate triggers for identities, identity_keys, dashpay_profiles, etc.; ensure
you update the for loop callers accordingly to pass either no cols (if removed)
or the appropriate slice like &["wallet_id"] or &["identity_id"] where needed.
packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs (1)

102-107: ⚡ Quick win

SafeCastTarget::U64 is misleading for a u32 overflow.

The cast here is i64 → u32, but the surfaced target is SafeCastTarget::U64. Operators reading the error will see "u64" and assume the value didn't fit in u64, when in fact it didn't fit in u32. Consider adding a U32 variant to SafeCastTarget (and a corresponding i64_to_u32 helper in safe_cast) so the error type accurately describes the target, and to avoid repeating this try_from pattern at every reader. This same issue recurs in core_state.rs for core_utxos.height and core_utxos.account_index.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs` around
lines 102 - 107, The IntegerOverflow error currently reports SafeCastTarget::U64
for an i64→u32 conversion in asset_locks.rs (see the u32::try_from usage
producing WalletStorageError::IntegerOverflow), which is misleading; add a U32
variant to crate::sqlite::util::safe_cast::SafeCastTarget and implement a
dedicated i64_to_u32 helper in the safe_cast module that performs the conversion
and returns WalletStorageError::IntegerOverflow with target =
SafeCastTarget::U32 on failure, then replace the manual u32::try_from usages
(e.g., the account_index conversion in asset_locks.rs and the similar
conversions in core_state.rs for core_utxos.height and core_utxos.account_index)
to call the new helper so errors accurately report the intended target type.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`:
- Around line 235-244: The current migrate branch uses peek_schema_version which
swallows all errors into None causing applied to be unreliable; change
peek_schema_version to return Result<Option<i64>, E> (i.e., Result<Option<i64>,
Box<dyn Error> or a concrete error type) and update the call site in the
Cmd::Migrate block: keep using the old behavior for pre_version (call
peek_schema_version before opening the DB if you must tolerate missing table),
but after SqlitePersister::open(config.clone()) call the new peek_schema_version
and propagate or error out on Err so the post_version probe cannot be silently
treated as 0; compute applied only when post_version is Ok(Some/_), and map
errors to the CLI error flow (use map_open_err_for_cli or similar) instead of
unwrap_or(0) so the printed "applied: N" is trustworthy.
- Around line 288-300: The CLI-level pre-check args.out.is_file() in run_backup
is redundant and does not prevent the TOCTOU race because backup_to() itself
checks exists() and ultimately calls run_to() -> Connection::open(dest) which
can be raced; either remove the args.out.is_file() guard from run_backup, or
(preferred) harden the persistence path by modifying backup_to()/run_to() to
perform atomic file creation (e.g., use OpenOptions::create_new() or equivalent)
when opening the destination instead of relying on exists()/Connection::open,
ensuring the creation fails if the file was created concurrently.

In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 517-520: The code that rebuilds state in load() uses
schema::platform_addrs::count_per_wallet and then inserts addrs into
state.platform_addresses only if count > 0 || addrs.sync_height > 0 ||
addrs.sync_timestamp > 0, which drops wallets whose only persisted platform
state is addrs.last_known_recent_block; update the reconstruction condition to
also check addrs.last_known_recent_block (e.g., include ||
addrs.last_known_recent_block > 0) so that entries with only
last_known_recent_block are preserved when inserting into
state.platform_addresses.
- Around line 412-468: flush_inner currently calls self.buffer.drain(wallet_id)
and discards the changeset (cs) before opening the DB transaction, so any
failure after draining (e.g., during schema::...::apply or tx.commit()) loses
the buffered changes; change the logic so the buffer is only removed after
tx.commit() succeeds: either fetch/clone/peek the changeset (use a
non-destructive read API instead of buffer.drain) into cs, execute the schema
apply calls and tx.commit(), and only then call buffer.drain(wallet_id) or
buffer.remove(wallet_id) to clear it; if you must drain early, ensure every
error path re-inserts/requeues the drained cs back into the buffer (e.g.,
buffer.insert(wallet_id, cs)) before returning the PersistenceError, referencing
flush_inner, self.buffer.drain(wallet_id), cs, and tx.commit() to locate the
edits.
- Around line 269-326: Before checking existence/backing up/deleting in
delete_wallet_inner, reconcile in-memory buffered writes for the target wallet:
call the component that persists or discards pending buffer entries (e.g.,
flush/commit_writes or discard_buffered_writes) for wallet_id before the initial
conn() existence check and before run_auto_backup; propagate any error from that
operation so the delete aborts on flush failures. Locate delete_wallet_inner and
insert the flush/discard call at the top (prior to the SELECT 1 and
run_auto_backup usages) and ensure the rest of the logic (PER_WALLET_TABLES
counting, schema::wallet_meta::delete, tx.commit) operates on the now-consistent
persisted state.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs`:
- Around line 26-28: The decode function currently ignores the bytes-consumed
result from bincode::serde::decode_from_slice, allowing trailing garbage to pass
as valid; update the blob::decode implementation (the decode function that calls
bincode::serde::decode_from_slice) to check the second tuple element
(bytes_consumed) against blob.len() and return a WalletStorageError (e.g.,
Err(WalletStorageError::CorruptedBlob) or similar existing error variant) when
bytes_consumed != blob.len(), otherwise return the decoded value; ensure the
error path uses the same error type returned on bincode failures to keep the
function signature consistent.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs`:
- Around line 108-136: The upsert_utxo function currently writes account_index
as 0 into core_utxos causing list_unspent_utxos to misassign UTXOs; fix by
populating account_index before inserting: either (a) perform a lookup/join
against core_derived_addresses inside upsert_utxo (match wallet_id +
script/address) to derive the correct account_index and use that value instead
of 0, or (b) modify the Utxo/CoreChangeSet pipeline so the writer that calls
upsert_utxo receives and passes the owning account_index, or (c) add a
deterministic backfill writer that runs after writes but before any
list_unspent_utxos calls to update core_utxos.account_index from
core_derived_addresses; update upsert_utxo (and any callers of
upsert_utxo/CoreChangeSet) to ensure account_index is never left as the
hardcoded 0.
- Around line 138-174: The read in upsert_sync_state currently uses lp.map(|x| x
as u32) / sy.map(|x| x as u32) which silently truncates out-of-range i64 values;
change the rusqlite row mapping to return Option<i64> for both heights (keep the
closure in query_row returning (Option<i64>, Option<i64>)), then after the
query_row.optional()? unwrap_or((None,None)) convert each Option<i64> to
Option<u32> using a checked helper (reuse i64_to_u64 + u32::try_from or add
i64_to_u32) and map conversion errors to WalletStorageError::IntegerOverflow so
the function returns an error rather than silently wrapping before using lp and
sy in the INSERT/ON CONFLICT params.
- Around line 27-45: The code iterates cs.spent_utxos and calls upsert_utxo(tx,
wallet_id, utxo, true) when the outpoint is missing, which will materialize a
synthetic UTXO with account_index = 0; document this intent at the call site or
add an explicit guard flag to make the exceptional behavior obvious. Update the
block in the loop that handles cs.spent_utxos to either (a) add a clear comment
above the else branch referencing upsert_utxo and why creating a spent-only
placeholder with account_index = 0 is safe and will be corrected later, or (b)
wrap the else branch in a condition/flag (e.g., only_insert_spent_placeholders)
and pass that flag through to upsert_utxo so callers must opt into creating
synthetic rows; reference cs.spent_utxos, upsert_utxo, and the
core_utxos/account_index = 0 behavior in the comment or the new guard.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 42-64: fetch currently only selects entry_blob and discards the
tombstoned column referenced in the doc; change fetch to SELECT entry_blob,
tombstoned from identities and return the tombstone flag alongside the decoded
IdentityEntry (e.g. update the signature to Result<Option<(IdentityEntry,
bool)>, WalletStorageError> or a small wrapper type), decode payload with
blob::decode for the entry, map the tombstoned SQL value to a bool, and update
the doc comment to reflect the new return shape (ensure you still use
rusqlite::OptionalExtension and propagate errors as before).

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs`:
- Around line 72-89: The upsert loop writes typed key columns from the loop key
((identity_id, key_id), wallet_id) but serializes the payload from entry (via
IdentityKeyWire::from_entry), which can produce mismatches; ensure the
serialized wire and the SQL key are consistent by either normalizing the wire
values to the loop key or rejecting mismatches before persistence: construct or
overwrite IdentityKeyWire fields using the loop's wallet_id/identity_id/key_id
(from cs.upserts key and wallet_id variable) prior to blob::encode, or validate
that entry.identity_id, entry.key_id and entry.wallet_id exactly match the loop
key and return an error if they differ, then proceed to tx.execute.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs`:
- Around line 19-37: The loop over cs.addresses currently ignores each entry's
own wallet_id and always writes using the outer wallet_id; add a fast-fail check
at the top of the loop that compares entry.wallet_id to the outer wallet_id (the
variables named entry and wallet_id in the cs.addresses loop) and return an
error if they differ (include a descriptive message mentioning the mismatched
wallet ids and the PlatformAddressBalanceEntry), before executing the INSERT;
this prevents silently writing mixed-wallet entries.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs`:
- Around line 45-51: The row mapper in the wallet_metadata reader must reject
malformed rows instead of coercing them: in the stmt.query_map closure that
reads wallet_id (currently into bytes and wid) and birth_height (converted with
as u32), validate that wallet_id.bytes.len() == 32 and that birth_height is
within u32 bounds before converting; if either check fails return a real error
(e.g., a StorageError::CorruptedRow or map a descriptive
rusqlite::Error::InvalidParameterName/Other) from the closure so the query fails
instead of producing a zeroed wallet id or truncated height. Locate the closure
that does row.get(0) / copy_from_slice and the code that casts birth_height to
u32 and replace the coercion with explicit checks and an early Err(...) return
with a typed, descriptive error.

In `@packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs`:
- Around line 87-106: The test's assertion that delete_wallet returns
AutoBackupDirUnwritable is flaky when run as root because chmod 0o500 doesn't
block root; modify the test in sqlite_auto_backup.rs (the block that creates
unwritable dir, calls SqlitePersisterConfig::with_auto_backup_dir,
SqlitePersister::open, and persister.delete_wallet) to detect running as root
(e.g., check geteuid()==0 on Unix) and in that case either skip the
exact-variant assertion or assert only that an error occurred, otherwise keep
the existing matches!(... AutoBackupDirUnwritable { .. }) check; this ensures
deterministic behavior without changing SqlitePersister/delete_wallet logic.

---

Nitpick comments:
In `@packages/rs-platform-wallet-storage/migrations/V001__initial.rs`:
- Around line 185-191: The comment mentions layering constraints/indexes "via
`inject_custom`" but the code builds raw DDL by concatenating into `tail` after
calling `m.make::<Sqlite>()`; either (A) change the implementation to use
Barrel's `inject_custom` API to append the constraints/indexes instead of manual
string concatenation (replace the `tail` concatenation and use
`m.inject_custom(...)` at the appropriate spot), or (B) update the comment to
accurately describe the current behavior (mention that raw DDL is appended to
`tail` after `m.make::<Sqlite>()` and that `inject_custom` is not used) so
future maintainers are not misled; locate references to `tail`,
`m.make::<Sqlite>()`, and any subsequent emission of the SQL to modify
accordingly.
- Around line 281-326: The closure parent_check currently accepts an unused
parameter _cols: &[&str]; either remove that parameter from parent_check and all
its callers (keep it as parent_check(child: &str) and continue using wallet_id
inside the generated trigger SQL), or update parent_check to accept cols:
&[&str] and use cols[0] (or join cols) in place of the hard-coded wallet_id
everywhere (including the WHEN clauses and DELETE WHERE clause) so the same
helper can generate triggers for identities, identity_keys, dashpay_profiles,
etc.; ensure you update the for loop callers accordingly to pass either no cols
(if removed) or the appropriate slice like &["wallet_id"] or &["identity_id"]
where needed.

In `@packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs`:
- Around line 188-231: The migrate handling is split across two places; fold the
two separate `if let Cmd::Migrate` blocks into one contiguous block so all
migrate-specific validation and invocation live together. Locate the existing
`if let Cmd::Migrate(m) = &cli.cmd {` usage, move the checks that validate
`auto_backup_dir` and the `m.no_auto_backup` mutation of `config` (the use of
`SqlitePersisterConfig::new(&db)` and `config.with_auto_backup_dir(...)`) into
the same block where the migrate command is executed (or replace both with a
single `run_migrate` helper that accepts `db`, `m`, `config`, and
`auto_backup_dir`), then perform the `run_migrate` call and return its Result
immediately; finally remove the now-unnecessary second `if let Cmd::Migrate` and
the `unreachable!()` arm.
- Around line 122-134: In parse_wallet_id, stop echoing the raw input in the
length error and validate/normalize hex consistently: when s.len() != 64 return
an Err that only mentions the expected length (do not include `s`), and replace
relying solely on hex::decode with an explicit check that all chars are
lowercase hex (0-9,a-f) so uppercase is rejected; keep the existing "wallet id
is not valid hex" error for decoding failures but ensure messages follow the
same non-leaking style.

In `@packages/rs-platform-wallet-storage/src/sqlite/config.rs`:
- Around line 111-118: The function default_auto_backup_dir contains an overly
defensive check when computing parent from db_path; simplify by using
db_path.parent().unwrap_or_else(|| Path::new(".")) to get a &Path fallback and
then join "backups"/"auto" off that parent. Update the local variable used
(currently named parent) to hold the &Path from parent() rather than mapping to
a PathBuf, and then call parent.join("backups").join("auto") to produce the
final PathBuf.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- Around line 102-107: The IntegerOverflow error currently reports
SafeCastTarget::U64 for an i64→u32 conversion in asset_locks.rs (see the
u32::try_from usage producing WalletStorageError::IntegerOverflow), which is
misleading; add a U32 variant to crate::sqlite::util::safe_cast::SafeCastTarget
and implement a dedicated i64_to_u32 helper in the safe_cast module that
performs the conversion and returns WalletStorageError::IntegerOverflow with
target = SafeCastTarget::U32 on failure, then replace the manual u32::try_from
usages (e.g., the account_index conversion in asset_locks.rs and the similar
conversions in core_state.rs for core_utxos.height and core_utxos.account_index)
to call the new helper so errors accurately report the intended target type.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs`:
- Around line 72-102: ensure_exists currently performs an immediate INSERT OR
IGNORE on a plain &Connection, which bypasses the in-memory merge buffer and
per-flush transaction used by apply and can cause durability/ordering
mismatches; update ensure_exists to participate in the same flush boundary by
either accepting and using the existing merge buffer/flush transaction or a
transactional handle (e.g., a &Transaction or buffer API used by apply) and
executing the INSERT within that transaction, or if intentionally intended only
for tests, add a clear doc comment on ensure_exists describing that it writes
immediately to the DB and must not be used by production code that relies on the
merge-buffer + per-flush transaction model (reference ensure_exists, apply, and
the in-memory merge buffer/flush transaction behavior in your comment).

In `@packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs`:
- Around line 20-26: The SafeCastTarget enum currently only lists I64 and U64
but callers need a U32 target; add a new variant SafeCastTarget::U32 and
implement a small helper function i64_to_u32 that mirrors existing
i64_to_u64/i64_to_i64 behavior (validate range, return Result<u32,
SafeCastError> and report SafeCastTarget::U32 on failure). Update any match/log
paths in this module that construct errors to use the new U32 variant so callers
like asset_locks::list_active and core_state::list_unspent_utxos can call the
helper and produce the correct error shape/message.

In `@packages/rs-platform-wallet-storage/tests/common/mod.rs`:
- Around line 47-56: The helper ensure_wallet_meta currently hardcodes network =
'testnet', which can collide with tests expecting other networks; change the
signature of ensure_wallet_meta(persister: &SqlitePersister, wallet_id:
&WalletId) to accept a network parameter (e.g., network: &str) with callers
passing "testnet" where appropriate, update the INSERT SQL call in
ensure_wallet_meta to bind the network parameter instead of the literal
'testnet', and update all test call sites (or add a convenience wrapper) to pass
the correct network values (e.g., "mainnet" or "testnet") so tests see
consistent wallet_metadata rows.

In `@packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs`:
- Around line 273-278: Remove the redundant dead-code shim `_unused_btreemap`
and its explanatory comment: delete the `#[allow(dead_code)] fn
_unused_btreemap() -> BTreeMap<u32, u32> { BTreeMap::new() }` block since
`BTreeMap` is now actually used in the test
`tc023_one_flush_is_one_transaction`, making the shim unnecessary; ensure
imports remain and run tests to confirm nothing else depends on that helper.
- Around line 154-180: Test currently calls fresh_persister() inside the
proptest closure causing a new on-disk SQLite DB per case; move persister
creation out of the proptest! Create the persister once before proptest (call
fresh_persister() once to get persister/_tmp/_path), then inside the proptest
closure use a new wallet id (wid) per case or clear/reset the core_sync_state
row (use ensure_wallet_meta and persister.store with core_with_height) so each
case reuses the same DB connection and only varies the wallet or row content;
update references to fresh_persister, wid, ensure_wallet_meta and persister
accordingly.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 78dea89d-cb94-44d0-8590-02ed68920ad8

📥 Commits

Reviewing files that changed from the base of the PR and between 3b9fe6b and d7a88a9.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (55)
  • .github/workflows/pr.yml
  • .github/workflows/tests-rs-workspace.yml
  • Cargo.toml
  • Dockerfile
  • packages/rs-platform-wallet-storage/Cargo.toml
  • packages/rs-platform-wallet-storage/README.md
  • packages/rs-platform-wallet-storage/SECRETS.md
  • packages/rs-platform-wallet-storage/build.rs
  • packages/rs-platform-wallet-storage/migrations/V001__initial.rs
  • packages/rs-platform-wallet-storage/migrations/V002__defensive_update_triggers.rs
  • packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs
  • packages/rs-platform-wallet-storage/src/lib.rs
  • packages/rs-platform-wallet-storage/src/sqlite/backup.rs
  • packages/rs-platform-wallet-storage/src/sqlite/buffer.rs
  • packages/rs-platform-wallet-storage/src/sqlite/config.rs
  • packages/rs-platform-wallet-storage/src/sqlite/error.rs
  • packages/rs-platform-wallet-storage/src/sqlite/migrations.rs
  • packages/rs-platform-wallet-storage/src/sqlite/mod.rs
  • packages/rs-platform-wallet-storage/src/sqlite/persister.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs
  • packages/rs-platform-wallet-storage/src/sqlite/util/mod.rs
  • packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs
  • packages/rs-platform-wallet-storage/tests/common/mod.rs
  • packages/rs-platform-wallet-storage/tests/secrets_scan.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_backup_restore.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs
  • packages/rs-platform-wallet/Cargo.toml
  • packages/rs-platform-wallet/src/changeset/changeset.rs
  • packages/rs-platform-wallet/src/changeset/mod.rs
  • packages/rs-platform-wallet/src/changeset/serde_adapters.rs
  • packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/block_time.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/dashpay/contact_request.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/dashpay/established_contact.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/dashpay/payment.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/dashpay/profile.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/key_storage.rs

Comment thread packages/rs-platform-wallet-storage/src/sqlite/backup.rs Outdated
Comment thread packages/rs-platform-wallet-storage/src/sqlite/persister.rs
Comment thread packages/rs-platform-wallet-storage/src/sqlite/persister.rs Outdated
Comment thread packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs
Comment thread packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs Outdated
Comment thread packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs Outdated
Comment thread packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs Outdated
Comment thread packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs Outdated
lklimek and others added 2 commits May 13, 2026 13:42
Routine forward-integration. Cargo.lock reconciliation only.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Comment-tightening pass per claudius:coding-best-practices, scoped to
PR #3625's own additions:

- sqlite_buffer_semantics.rs: drop `_unused_btreemap` placeholder + its
  "future expansion" comment. `BTreeMap` is genuinely used elsewhere in
  the file (line 301 — `balances` map), so the import stays. Removes a
  speculative-future-state comment and an empty helper that exists only
  to silence a phantom lint.
- sqlite_load_reconstruction.rs: fix stale cross-reference. Module doc
  said "tracked in a TODO in persister.rs::load", but the actual signal
  is the `LOAD_UNIMPLEMENTED` constant + tracing::warn. Replace with the
  accurate present-state pointer.

Plus a single rustfmt fix in
`packages/rs-platform-wallet/src/wallet/platform_addresses/wallet.rs`
that fell out of the v3.1-dev merge — the textual auto-merge produced a
3-arg call spread across 5 lines that rustfmt collapses to one line.
Not a logic change.

Rules driving the changes:
- present-state, not history (sqlite_load_reconstruction.rs)
- comment only when meaningful — dropping speculative placeholders
  (sqlite_buffer_semantics.rs)

Quality gates: `cargo fmt --all` clean, `cargo check --workspace` green,
`cargo clippy -p platform-wallet -p platform-wallet-storage --tests
--no-deps -- -D warnings` green.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…reation

SEC-011 (Smythe audit, MEDIUM): the restore path already applied `chmod
0o600` after writing the SQLite file (`backup.rs::restore_from`), but
the initial-create path in `SqlitePersister::open` and the
backup-create path in `backup::run_to` did not. Both relied on the
process umask, which can leave a newly created DB world- or
group-readable.

Extracts the existing inline `#[cfg(unix)]` + `Permissions::from_mode(0o600)`
block into a small helper `sqlite::util::permissions::apply_secure_permissions`
(no-op on non-Unix) and calls it at all three sites. The restore path
keeps its existing semantics — it just delegates to the helper now —
so the file mode no longer depends on the process umask anywhere a
SQLite file is created or replaced by this crate.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@Claudius-Maginificent
Copy link
Copy Markdown
Collaborator Author

SEC-011 fix landed (f6e90d1fca)

Out-of-band security audit (by an automated agent surveying the sqlite persister's secret-handling posture) flagged that the restore path applies chmod 0o600 to the staged SQLite file but the initial DB creation and backup creation paths don't — files land at the process umask, which on shared systems could be world-readable.

The audit classified this as MEDIUM under the umbrella "SEC-011: missing explicit 0o600 on initial DB + backup creation".

Fix shape: extracted a small apply_secure_permissions(&Path) -> Result<(), WalletStorageError> helper in a new packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs (Unix-gated via #[cfg(unix)], mode 0o600, propagates I/O errors through the existing WalletStorageError: #[from] std::io::Error). Called at all three sites:

  • persister.rs:108 after Connection::open(&config.path) — initial DB
  • backup.rs:48 after Connection::open(dest) in run_to — backup
  • backup.rs restore_from — refactored from the existing inline pattern to call the helper

Net change: 4 files, +35/-7 lines. cargo fmt, cargo check --tests, cargo clippy --tests -D warnings, and cargo test --lib (10/10) all clean.

Companion privacy finding flagged in the same audit pass (SEC-012: accounts.account_xpub_bytes enables tx-graph reconstruction if the SQLite file is exfiltrated) — that's a documentation/policy decision rather than a code fix. Best handled in a follow-up SECRETS.md update noting the privacy trade-off explicitly.

🤖 Co-authored by Claudius the Magnificent AI Agent

Claudius-Maginificent and others added 3 commits May 18, 2026 19:40
…e_cached writers, functional load() (#3643)

Co-authored-by: Lukasz Klimek <842586+lklimek@users.noreply.github.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…3633)

Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-authored-by: QuantumExplorer <quantum@dash.org>
lklimek and others added 8 commits May 25, 2026 10:55
… failure (CMT-002 data-loss)

delete_wallet_inner drained the per-wallet buffered changeset BEFORE
the existence check / auto-backup / DELETE transaction, but never
restored it on any pre-commit failure. The operator's delete correctly
reported the backup or SQL error, yet the wallet's pending writes
vanished from the buffer even though no DELETE actually committed.

Mirror flush_inner's take/restore discipline: hold the drained
changeset in a Cell<Option<...>>, on success consume it AFTER
tx.commit(), on any error path restore it into the buffer via
self.buffer.restore(wallet_id, cs). If the restore itself fails (e.g.
poisoned buffer mutex), log the lost changeset at error level — the
delete error still surfaces (it's the primary cause).

Regression: tests/sqlite_delete_wallet::delete_wallet_restores_buffer_on_backup_failure
points auto_backup_dir at an unwritable path, primes a buffered
changeset, asserts (a) delete returns AutoBackupDirUnwritable and
(b) the buffered writes survive — a follow-up flush lands them.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…ic with restore_from (CMT-005)

SqlitePersister::open ran the migration probe (count_pending) and
refinery::run, but never compared the on-disk MAX(version) against the
highest embedded migration. An older binary opening a DB produced by a
newer release saw pending_count == 0 and silently proceeded to decode
forward-schema blob columns — the migration version IS the entire
compatibility boundary for this crate (per blob.rs:4-6).

Add the same SchemaVersionUnsupported gate restore_from enforces,
calling the shared migrations::assert_schema_version_supported helper
introduced in the CMT-010 commit. Sites are now symmetric.

Regression: tests/sqlite_open_version_gate::open_rejects_forward_schema_version
forges a version=1_000_000 row, asserts the second open fails with
SchemaVersionUnsupported.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…ent stores (CMT-008)

delete_wallet_inner previously released and re-acquired the connection
Mutex three times (existence-check / auto-backup / DELETE-tx). A
parallel store(wallet_id, cs) could slip in between any pair of those
scopes — in Immediate mode the racing flush landed AFTER the DELETE
commit (wallet reappears), in Manual mode a buffered store survived
the delete and was persisted by the next commit_writes() against a
freshly-recreated parent.

Approach (a): acquire self.conn()? ONCE at the top of
delete_wallet_inner and hold it across the full pipeline so Immediate
stores block on flush_inner's own self.conn(). For Manual mode (where
store doesn't take conn at all), do a post-commit second drain of the
buffer to discard any racing-buffered changeset — the wallet is being
removed, those writes are intentionally void. run_auto_backup already
accepted &Connection so no signature changes were required.

Regression: tests/sqlite_delete_wallet::concurrent_store_does_not_resurrect_deleted_wallet
spawns a worker hammering store() while the main thread deletes,
commits any remaining writes, asserts every PER_WALLET_TABLES row
count for that wallet_id is zero.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…e / typed AssetLockEntryMismatch (CMT-006/007)

CMT-006: blob::decode used bincode's NoLimit config, so an attacker-
controlled length prefix inside a Vec/String/map could drive the
allocator BEFORE the trailing-byte gate ever fired — a crafted backup
that passed restore_from's integrity check could OOM the host on the
next load(). Switch encode + decode to .with_limit::<16 MiB>() (well
above any legitimate per-row payload) and surface the cap-exceeded
case as a NEW typed variant `WalletStorageError::BlobTooLarge {
len_bytes, limit_bytes }` so operators can distinguish hostile/
corrupt oversize from a structural decode failure. Generic decode
failures (trailing bytes, schema mismatch) still surface as the
existing `BlobDecode` per user preference for precise variants.

CMT-007: asset_locks::decode_row decoded the typed outpoint from
op_bytes but then constructed TrackedAssetLock using the BLOB's
out_point + account_index. A torn write or restored corruption that
diverged blob-vs-typed would silently mis-bucket the lock under the
wrong account. Add a cross-check symmetric with IdentityKeyEntryMismatch
under a NEW typed variant `WalletStorageError::AssetLockEntryMismatch {
typed_outpoint, blob_outpoint, typed_account_index, blob_account_index }`.

is_transient() + error_kind_str() updated for both new variants — the
match remains wildcard-free per the existing #[non_exhaustive]-rejection
note in error.rs.

Regressions:
- src/sqlite/schema/blob.rs::tests::decode_rejects_oversize_blob_with_blob_too_large
- tests/sqlite_hardening_3625::asset_lock_typed_vs_blob_mismatch_rejected

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…inner blob (CMT-009)

IdentityKeyWire::into_entry decoded public_key_bincode with
bincode::decode_from_slice but bound the consumed count to `_`. A
corrupt or forward-incompatible inner payload whose IdentityPublicKey
prefix is valid would deserialize successfully even with garbage past
the typed length — leaving identity_keys weaker than every other blob
path. The outer blob::decode already enforces the same gate.

Capture consumed and surface BlobDecode("unexpected trailing bytes in
identity_keys.public_key_bincode") on mismatch (using the existing
blob_decode helper per user preference — trailing-bytes is the
canonical BlobDecode case, not a typed-variant case).

Regression: src/sqlite/schema/identity_keys::tests::into_entry_rejects_trailing_bytes_in_public_key_bincode

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…te (CMT-011)

list_active was byte-for-byte identical to load_state — same SQL,
same decode loop. Per the docstring's own admission, every persisted
status is considered active (consumed locks leave via
AssetLockChangeSet::removed), so the missing WHERE clause was
intentional but made the two functions semantically indistinguishable.

Option (a) per user preference: drop list_active, migrate the one
caller (tests/sqlite_persist_roundtrip.rs:377) to load_state, refresh
load_state's docstring to subsume the consumed-locks comment, and
verify zero remaining references with grep.

Also patches the wildcard-free match in tests/sqlite_error_classification.rs
to cover the new AssetLockEntryMismatch / BlobTooLarge variants added
in the prior CMT-006/007 commit (the exhaustiveness gate is what
this test exists to enforce).

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…MT-012)

The smoke test previously accepted any non-success via
assert!(!out.status.success(), ...) so a runtime failure unrelated to
unknown-subcommand handling (panic during clap setup, OOM, signal
kill) would still pass. Assert the explicit clap usage code Some(2)
before checking stderr content — costs nothing, tightens the contract.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
- sqlite_delete_wallet.rs: drop a useless std::fs::Permissions::from(...).
- sqlite_permissions.rs: silence clippy::field_reassign_with_default
  (consistent with the existing test files' top-level allow).

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
@lklimek lklimek dismissed thepastaclaw’s stale review May 25, 2026 09:52

Addressed in 614c43d (10 commits): all 12 verified findings fixed (2 BLOCKING data-loss, 2 HIGH security, 6 MEDIUM, 2 LOW). Marvin QA GREEN, 114/0/1 tests.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

I rechecked all 12 mandatory prior findings from the df1e59ca review against head 614c43d4b0ca00001e2934506682095595d9f202; every one of those prior findings is FIXED, so there are no carried-forward mandatory prior findings. The latest cumulative head still has three blocking issues in rs-platform-wallet-storage that are not covered by that resolved set, plus one suggestion-level cleanup issue in the backup path. No CodeRabbit findings were provided.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 614c43d
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 138, in
result = post_review(repo, pr_number, h), so I posted the same verified findings as a top-level review body._

Reviewed commit: 614c43d

🔴 3 blocking | 🟡 1 suggestion(s)

Verified findings

blocking: Restore releases the destination lock before the destructive replace starts

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 127)

restore_from() uses try_lock_exclusive() only as a probe, then immediately unlocks and drops the file handle before staging, sidecar removal, and persist(). That leaves a TOCTOU window where another process can open and start using the destination database after the check passes but before the restore replaces it. In that state RestoreDestinationLocked is not actually protecting the live database, and the restore can overwrite a database that became active during the window.

blocking: Restore deletes live WAL/SHM sidecars before the replacement rename is known to have succeeded

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 185)

The staged-copy validation now happens before sidecar deletion, but the restore still unlinks <dest>-wal and <dest>-shm before tmp.persist(dest_db_path) succeeds. If that final rename fails, the original main database file remains in place while any committed pages that only existed in the WAL have already been discarded. That is a real data-loss path on the live destination, so the current sequence is not atomic.

blocking: Successful writes still leave newly created WAL/SHM sidecars at the process umask

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 500)

apply_secure_permissions() only chmods sidecars that already exist, and SqlitePersister::open() runs it before any write has created wallet.db-wal or wallet.db-shm. write_changeset_in_one_tx() then commits and returns without reapplying permissions. In WAL mode those sidecars are commonly created lazily on the first write, so they can remain world- or group-readable even though the main database file is forced to 0600. On a multi-user host that exposes recent wallet pages directly from the sidecars.

        tx.commit()?;
        apply_secure_permissions(&self.config.path)?;
        Ok(())
suggestion: Failed backups leave a destination file behind and make retries fail with BackupDestinationExists

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 54)

run_to() creates dest up front with create_new(true), then returns early on any later failure from open_conn, apply_secure_permissions, Backup::new, or run_to_completion without removing the file it just created. That leaves behind an empty or partial backup artifact, and a retry to the same explicit destination will fail immediately with BackupDestinationExists instead of retrying cleanly. The stale file is user-visible and turns transient backup errors into sticky operational failures.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:127-145: Restore releases the destination lock before the destructive replace starts
  `restore_from()` uses `try_lock_exclusive()` only as a probe, then immediately unlocks and drops the file handle before staging, sidecar removal, and `persist()`. That leaves a TOCTOU window where another process can open and start using the destination database after the check passes but before the restore replaces it. In that state `RestoreDestinationLocked` is not actually protecting the live database, and the restore can overwrite a database that became active during the window.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:185-205: Restore deletes live WAL/SHM sidecars before the replacement rename is known to have succeeded
  The staged-copy validation now happens before sidecar deletion, but the restore still unlinks `<dest>-wal` and `<dest>-shm` before `tmp.persist(dest_db_path)` succeeds. If that final rename fails, the original main database file remains in place while any committed pages that only existed in the WAL have already been discarded. That is a real data-loss path on the live destination, so the current sequence is not atomic.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:500-546: Successful writes still leave newly created WAL/SHM sidecars at the process umask
  `apply_secure_permissions()` only chmods sidecars that already exist, and `SqlitePersister::open()` runs it before any write has created `wallet.db-wal` or `wallet.db-shm`. `write_changeset_in_one_tx()` then commits and returns without reapplying permissions. In WAL mode those sidecars are commonly created lazily on the first write, so they can remain world- or group-readable even though the main database file is forced to `0600`. On a multi-user host that exposes recent wallet pages directly from the sidecars.
- [SUGGESTION] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:54-92: Failed backups leave a destination file behind and make retries fail with BackupDestinationExists
  `run_to()` creates `dest` up front with `create_new(true)`, then returns early on any later failure from `open_conn`, `apply_secure_permissions`, `Backup::new`, or `run_to_completion` without removing the file it just created. That leaves behind an empty or partial backup artifact, and a retry to the same explicit destination will fail immediately with `BackupDestinationExists` instead of retrying cleanly. The stale file is user-visible and turns transient backup errors into sticky operational failures.

lklimek and others added 11 commits May 25, 2026 13:38
… staging (ATOM-004 / A-1)

The pre-A-1 path eagerly created an empty SQLite file at `dest` via
`OpenOptions::create_new`, so any later failure (chmod, Backup::new,
run_to_completion) stranded a partial/empty .db at the caller-supplied
path. For auto-backup callers that's retention-dir pollution; for the
CLI's manual `backup_to(file_path)` it's junk at a user path with no
cleanup.

Stage the page-step copy into a `NamedTempFile` in `dest`'s parent
(same pattern `restore_from` already uses). chmod 600 the temp BEFORE
persist so the destination inherits owner-only mode via the atomic
rename — no umask-default window. The temp drops naturally on any
error before persist.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…od temp before persist (ATOM-005/010 / A-2 / A-5)

A-2: the try-lock arm probed the destination lock, dropped the handle
immediately, then ran stages 4-7 (WAL/SHM unlink, stage, validate,
persist) unlocked. A concurrent opener between probe and persist could
race the rename and end up holding a fd against the unlinked old
inode. Hold the exclusive lock guard across the entire restore body;
emit a structured `tracing::warn` on filesystems where flock is
unsupported instead of silently bypassing the safety net.

A-5: chmod the temp file to 0o600 BEFORE persist so the destination
inherits owner-only mode via atomic rename. Pre-A-5 the chmod ran
post-persist — a rare chmod failure left the new DB live while the
function returned Err, giving the caller a mixed result.

N-6: lifts the atomicity-guarantee paragraph from inline comments
into the public rustdoc of `restore_from`.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…per-wallet errors (N-1)

The old `commit_writes` iterated dirty wallets with `?` — the first
per-wallet failure aborted the loop. Wallets ordered after stayed
buffered with no caller-visible signal of which ones, leading to
silent data left in memory after the operator thinks the commit ran.

Approach (a) from the spec: return `Result<CommitReport, _>` carrying
`succeeded` / `failed` / `still_pending` per-wallet outcomes. The
trait `PlatformWalletPersistence` is untouched — `commit_writes` is a
`SqlitePersister`-specific method (not in the trait, no FFI consumer)
so the change is local to in-crate callers.

A `LockPoisoned` short-circuit on the connection mutex still bails
early, shoveling the remaining wallets into `still_pending` rather
than attempting flushes that would all hit the same dead mutex.

Includes regression test ATOM-006 that primes 3 dirty wallets, forces
A to fail fatally, and verifies B + C still flush and the report
captures the partition correctly.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…Manual mode (N-2 / ATOM-007)

A `SqlitePersister` dropped in `FlushMode::Manual` silently destroyed
every dirty wallet's buffered changeset — the trait has no
`commit_writes` and the buffer dies with the persister. The footgun
surfaced nowhere in the operator logs.

Add `impl Drop` that emits `tracing::error!` with `dirty_wallets` and
`total_fields` structured fields when dropped Manual-mode with a
non-empty buffer. Do NOT auto-flush — flushing can fail, Drop can't
propagate, and a swallow on flush would be a worse failure mode than
the loud log. Immediate-mode persisters skip the check entirely
(every `store` is durable, so the buffer is always empty at drop).

Regression tests assert both the Manual-mode log emits AND the
Immediate-mode path stays silent.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…SQLite codes (ATOM-008 / A-4)

Pre-A-4 only `DatabaseBusy` / `DatabaseLocked` were classified as
transient. Operational disk-full (`SQLITE_FULL`), I/O failures
(`SQLITE_IOERR`), and memory pressure (`SQLITE_NOMEM`) are also
recoverable environmental conditions — the operator clears disk /
the kernel finishes its blip / memory frees up — and treating them as
fatal drops buffered state for a problem that resolves itself.

Add `DiskFull`, `SystemIoFailure`, `OutOfMemory` to the transient
set. Extend `error_kind_str` with matching tags
(`sqlite_disk_full` / `sqlite_io_failure` / `sqlite_out_of_memory`)
so operators can grep production logs. Outer match on
`WalletStorageError` stays wildcard-free; inner `ErrorCode` match
uses `_` because the upstream type is `#[non_exhaustive]`. Updates
the classification test with one sample per new code.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…ors (ATOM-011 / A-6)

Pre-A-6 the loop did `remove_file(&path)?` so the 4th of 7 files
failing left 1-3 gone, 4 errored, 5-7 untouched, and the caller saw an
io::Error with no record of partial progress. The `INTENTIONAL(CODE-007)`
comment acknowledged the limitation; we now address it.

Add `PruneReport::failed_removals: Vec<(PathBuf, io::Error)>` and
collect per-file failures instead of bailing. Catastrophic errors
(`read_dir` itself fails) still surface as `Err`. CLI surfaces failed
removals to stderr and exits non-zero so scripts catch partial
success.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…pen (ATOM-013 / A-8)

Pre-A-8 `open()` ran migrations against the live DB without first
asserting integrity. Bit-rot or escaped-WAL corruption gets migrated,
and the pre-migration auto-backup snapshots the corrupt state —
making rollback useless.

Add a `PRAGMA integrity_check` between `apply_pragmas` and the
migration run, gated on `had_schema_history` (a freshly-created file
has no btree pages to verify). Surfaces as the typed
`IntegrityCheckFailed { report }` (variant already existed for the
restore_from path) before any schema mutation lands. Promotes
`run_integrity_check` to `pub(crate)` so the open path shares the
same helper as restore_from.

Includes regression test that opens a fresh DB, mutates btree pages
past the SQLite header, closes, and re-opens — assertion that the
typed `IntegrityCheckFailed` surfaces.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…variant (N-5)

`WalletStorageError::MigrationDirty` was declared (with rustdoc),
routed through `is_transient` and `error_kind_str`, sample-listed in
the classification test — but never constructed anywhere in the
crate. Refinery surfaces partial migrations as
`Migration(refinery::Error)`, so the dedicated variant is dead code.

Drop the variant + its arms in `is_transient` / `error_kind_str` /
the classification test's sample list and inner match. The outer
match stays wildcard-free.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…est-effort (A-7)

Approach (b) from the spec: keep the writability probe — it catches
the common operator-misconfiguration cases cheaply — but document its
TOCTOU nature explicitly and note that the actual `run_to` below
carries its own typed error path for the post-probe failure window.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…7/N-9/N-10)

Surface implementation atomicity guarantees on the caller-visible
rustdoc that the Nagatha audit flagged as missing:

- N-3: `PlatformWalletPersistence::flush` trait method gets an
  `# Errors` section enumerating the transient-vs-fatal contract
  (transient -> buffer restored, fatal -> buffer dropped + error
  logged at the persister), cross-referencing ATOM-008's I/O-class
  additions.
- N-4: `SqlitePersister::delete_wallet` rustdoc explicitly calls out
  the racing-store discard semantics. A concurrent `store` may
  return `Ok(())` but its data does not survive the delete.
- N-7: `impl PlatformWalletPersistence for SqlitePersister`'s `store`
  spells out the FlushMode::Immediate (durable on Ok) vs Manual
  (buffer-only) durability matrix.
- N-9: `SqlitePersister::open` gets the standard `# Errors` taxonomy
  including the new `IntegrityCheckFailed` introduced in ATOM-013.
- N-10: `load`'s rustdoc gets a Concurrency section noting it holds
  the connection mutex for the whole read.

(N-6 already landed in the A-2/A-5 commit on `restore_from`.)

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
…k operator note

Two trivia from Marvin's atomicity QA pass.

- `impl Drop for SqlitePersister`: the `Drop` body calls
  `buffer.take_for_flush(id)` to drain each dirty wallet's changeset so
  `populated_field_count` can produce the diagnostic. Adds a comment
  noting the intentional side-effect so future maintainers don't treat
  `impl Drop` as side-effect-free.

- README operational notes: documents the `restore` advisory-lock
  warning emitted on filesystems without flock support (NFS / FUSE /
  network mounts), with an operator-facing recommendation. Same
  section also documents the Manual-mode drop diagnostic.

No behavioral changes. Doc + comment only.

Co-Authored-By: Claudius the Magnificent (1M context) <noreply@anthropic.com>
@Claudius-Maginificent
Copy link
Copy Markdown
Collaborator Author

🛡️ Atomicity hardening — 614c43d4b0..fe01634684 (11 new commits)

Combined contract audit (Nagatha) + implementation drill-down (Adams) flagged 12 atomicity findings on top of the prior CMT-* hardening pass. All 12 landed in 10 atomicity commits plus 1 polish commit. Marvin QA: GREEN, 0 CRITICAL / 0 HIGH / 0 MEDIUM findings.

Findings closed

Severity ID Headline Commit
HIGH A-1 backup::run_to atomic via NamedTempFile staging — no half-DB on partial failure b117ff2ef1
HIGH A-2 / A-5 / N-6 restore_from holds exclusive lock across body + chmod-before-persist + # Atomicity rustdoc 9b39783f87
HIGH N-1 commit_writes returns CommitReport { succeeded, failed, still_pending } — continues past per-wallet failures instead of first-fail-bail ae9e629577
HIGH N-2 / A-3 impl Drop for SqlitePersister logs uncommitted dirty buffer in Manual mode (no auto-flush) fb3362e233
MEDIUM A-4 is_transient expanded to cover I/O-class SQLite codes (DiskFull / SystemIoFailure / OutOfMemory) a23afd9277
MEDIUM A-6 backup::prune accumulates per-file errors into PruneReport::failed_removals; CLI exits 1 on partial failure df54420482
MEDIUM A-8 integrity_check before migrations on open() — corruption gated before migration mutations df76e1a6f6
MEDIUM N-5 Removed unreachable WalletStorageError::MigrationDirty variant c0b45c0d46
LOW A-7 ensure_dir TOCTOU probe documented as best-effort 1840b35905
(docs) N-3 / N-4 / N-7 / N-9 / N-10 Rustdoc tightening: flush failure contract, delete_wallet racing-store, store Immediate/Manual durability, open errors taxonomy, load conn-mutex disclosure 722de93647
(polish) Marvin LOW Drop side-effect comment + README operational notes (restore-lock warning, Manual-mode drop diagnostic) fe01634684

⚠️ Breaking API change

SqlitePersister::commit_writes signature changed:

// before
pub fn commit_writes(&self) -> Result<(), PersistenceError>

// after
pub fn commit_writes(&self) -> Result<CommitReport, PersistenceError>

where CommitReport { succeeded: Vec<WalletId>, failed: Vec<(WalletId, PersistenceError)>, still_pending: Vec<WalletId> }.

Scope: this method is on SqlitePersister directly, NOT in the PlatformWalletPersistence trait. In-tree consumers are migrated (4 test sites). No FFI ripple. Out-of-tree consumers calling commit_writes will need to update — the new return type carries strictly more information so the migration is mechanical:

  • Old ?-propagation: replace with let report = ?; if !report.failed.is_empty() { /* surface */ }.
  • Old "Ok = all wallets durable" assumption: now wrong by construction — report.failed may be non-empty even on Ok, and report.still_pending lists wallets that need re-flush after a LockPoisoned short-circuit.

Verification

  • cargo test -p platform-wallet-storage — 21 test binaries, 0 failed, 1 doc-test ignored (lib.rs example placeholder)
  • cargo clippy -p platform-wallet -p platform-wallet-storage -p platform-wallet-ffi --all-targets -- -D warnings — clean
  • cargo check --workspace --all-features — clean
  • 3 new test files: sqlite_open_integrity_check.rs (A-8 corruption test), additions to sqlite_buffer_semantics.rs (atom_006_commit_writes_continues_past_per_wallet_failures, Drop tests via tracing_test::traced_test), additions to sqlite_error_classification.rs (I/O code samples)

Net delta

12 files changed, 702 insertions(+), 96 deletions(-)

Confined to packages/rs-platform-wallet-storage/{src,tests} + one targeted doc add on packages/rs-platform-wallet/src/changeset/traits.rs (N-3 flush doc — no behavior change).

🤖 Co-authored by Claudius the Magnificent AI Agent

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Against head 722de93647478ba4b10483a3b12e3b58e5f6735a, the four prior storage findings are FIXED: backup creation now stages through a temp file, restore now holds the destination lock through the replace, restore chmods before persist, and open() runs integrity_check before migrations. Of the four older FFI findings, two are STILL VALID and carried forward (load() and get_core_tx_record() still accept host allocations without requiring the paired free callback), the callback-context ownership finding is INTENTIONALLY DEFERRED because the ABI explicitly makes that lifetime caller-owned and the in-tree Swift manager retains the handler, and the panic-unwind finding is OUTDATED because extern "C" panics abort instead of unwinding across the foreign boundary. The latest delta itself introduces two new blocking races in the restore and delete auto-backup paths: both take their rollback snapshot before any cross-process exclusion, so a concurrent writer can commit state that is later destroyed but absent from the advertised safety backup.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 722de93
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 138, in
result = post_review(repo, pr_number, h), so I posted the same verified findings as a top-level review body._

Reviewed commit: 722de93

🔴 2 blocking | 🟡 2 suggestion(s)

Verified findings

blocking: New finding in latest delta: pre-restore auto-backup is taken before any cross-process lock, so rollback can miss concurrent commits

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 297)

restore_from_inner() creates the pre-restore-*.db snapshot at lines 297-313, then calls backup::restore_from() at line 315, and that function does not acquire the destination file lock until later in backup.rs. Another process with its own SQLite connection can commit changes after the snapshot finishes but before the restore path locks and replaces the database, so the live database loses those rows while the auto-backup never captured them. That breaks the function's documented safe-by-default rollback guarantee for exactly the destructive case the backup is supposed to protect.

blocking: New finding in latest delta: `delete_wallet` snapshots before any cross-process exclusion, so the pre-delete backup is not a reliable rollback point

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 381)

delete_wallet_inner() holds only the in-process connection mutex while it calls run_auto_backup() at lines 423-428, and it does not begin the write transaction until line 430. A second process using a separate SQLite connection is not covered by that mutex and can commit wallet rows after the snapshot completes but before this transaction obtains the write lock and cascades the delete. Those rows are removed from the live database but never exist in pre-delete-*.db, so the advertised auto-backup can silently fail to preserve the pre-destructive state.

suggestion: Carried-forward prior finding: `load()` accepts host-owned wallet-list buffers without requiring the paired free callback

packages/rs-platform-wallet-ffi/src/persistence.rs (line 1284)

The callback contract at lines 157-172 says on_load_wallet_list_fn returns a host-allocated WalletRestoreEntryFFI array that Rust must release with on_load_wallet_list_free_fn, but FFIPersister::load() only checks that the loader exists. If a non-Swift consumer wires on_load_wallet_list_fn without the matching free callback, LoadGuard becomes a no-op and every successful restore leaks the host allocation. The in-tree Swift implementation pairs these callbacks correctly, but the exported ABI does not enforce that invariant.

        let Some(load_cb) = self.callbacks.on_load_wallet_list_fn else {
            return Ok(ClientStartState::default());
        };
        if self.callbacks.on_load_wallet_list_free_fn.is_none() {
            return Err(
                "on_load_wallet_list_fn and on_load_wallet_list_free_fn must be provided together"
                    .to_string()
                    .into(),
            );
        }

suggestion: Carried-forward prior finding: `get_core_tx_record()` accepts tx-byte buffers without requiring the paired free callback

packages/rs-platform-wallet-ffi/src/persistence.rs (line 1541)

The callback contract at lines 432-453 says a hit from on_get_core_tx_record_fn may return a host-allocated out_tx_bytes buffer that Rust releases through on_get_core_tx_record_free_fn, but get_core_tx_record() never validates that the free callback is present before constructing TxBytesGuard. If a consumer returns non-null transaction bytes with on_get_core_tx_record_free_fn == None, every successful lookup leaks the host allocation. The shipped Swift bridge wires the pair correctly today, but the public ABI still accepts the half-configured state.

        let Some(get_cb) = self.callbacks.on_get_core_tx_record_fn else {
            return Ok(None);
        };
        if self.callbacks.on_get_core_tx_record_free_fn.is_none() {
            return Err(
                "on_get_core_tx_record_fn and on_get_core_tx_record_free_fn must be provided together"
                    .to_string()
                    .into(),
            );
        }

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:297-315: New finding in latest delta: pre-restore auto-backup is taken before any cross-process lock, so rollback can miss concurrent commits
  `restore_from_inner()` creates the `pre-restore-*.db` snapshot at lines 297-313, then calls `backup::restore_from()` at line 315, and that function does not acquire the destination file lock until later in `backup.rs`. Another process with its own SQLite connection can commit changes after the snapshot finishes but before the restore path locks and replaces the database, so the live database loses those rows while the auto-backup never captured them. That breaks the function's documented safe-by-default rollback guarantee for exactly the destructive case the backup is supposed to protect.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:381-430: New finding in latest delta: `delete_wallet` snapshots before any cross-process exclusion, so the pre-delete backup is not a reliable rollback point
  `delete_wallet_inner()` holds only the in-process connection mutex while it calls `run_auto_backup()` at lines 423-428, and it does not begin the write transaction until line 430. A second process using a separate SQLite connection is not covered by that mutex and can commit wallet rows after the snapshot completes but before this transaction obtains the write lock and cascades the delete. Those rows are removed from the live database but never exist in `pre-delete-*.db`, so the advertised auto-backup can silently fail to preserve the pre-destructive state.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:1284-1300: Carried-forward prior finding: `load()` accepts host-owned wallet-list buffers without requiring the paired free callback
  The callback contract at lines 157-172 says `on_load_wallet_list_fn` returns a host-allocated `WalletRestoreEntryFFI` array that Rust must release with `on_load_wallet_list_free_fn`, but `FFIPersister::load()` only checks that the loader exists. If a non-Swift consumer wires `on_load_wallet_list_fn` without the matching free callback, `LoadGuard` becomes a no-op and every successful restore leaks the host allocation. The in-tree Swift implementation pairs these callbacks correctly, but the exported ABI does not enforce that invariant.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:1541-1602: Carried-forward prior finding: `get_core_tx_record()` accepts tx-byte buffers without requiring the paired free callback
  The callback contract at lines 432-453 says a hit from `on_get_core_tx_record_fn` may return a host-allocated `out_tx_bytes` buffer that Rust releases through `on_get_core_tx_record_free_fn`, but `get_core_tx_record()` never validates that the free callback is present before constructing `TxBytesGuard`. If a consumer returns non-null transaction bytes with `on_get_core_tx_record_free_fn == None`, every successful lookup leaks the host allocation. The shipped Swift bridge wires the pair correctly today, but the public ABI still accepts the half-configured state.

@lklimek
Copy link
Copy Markdown
Contributor

lklimek commented May 25, 2026

@coderabbitai review all

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 25, 2026

@lklimek Sure! I'll kick off a fresh full review covering all the latest changes, including the SEC-011 secure permissions fix and the atomicity hardening commits.

✅ Actions performed

Full review triggered.

Copy link
Copy Markdown
Collaborator

@thepastaclaw thepastaclaw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

Rechecked all 4 mandatory prior findings from the 722de93 review against head fe01634: all 4 are FIXED. The latest delta (722de936..fe016346) only adds README text and a comment in persister.rs, and it does not introduce any new findings. This review still requests changes because the current head carries 6 cumulative issues outside that prior-four set: 4 blocking correctness/data-safety bugs and 2 FFI memory-leak contract bugs.

_Note: Inline posting failed (command failed (1): python3 scripts/review_poster.py dashpay/platform 3625 fe01634
STDOUT:

STDERR:
Traceback (most recent call last):
File "/Users/claw/.openclaw/workspace/scripts/review_poster.py", line 138, in
result = post_review(repo, pr_number, h), so I posted the same verified findings as a top-level review body._

Reviewed commit: fe01634

🔴 4 blocking | 🟡 2 suggestion(s)

Verified findings

blocking: `restore_from()` treats every non-`WouldBlock` lock failure as "locking unsupported" and proceeds unlocked

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 187)

The restore path only handles WouldBlock as a hard error. Every other try_lock_exclusive() failure falls into Err(_), logs "advisory lock unsupported on this filesystem", and continues with the destructive swap. That branch is too broad: real lock-acquisition failures such as permission errors or I/O failures are silently downgraded into an unsupported-filesystem warning, so the restore can overwrite the destination without the protection the code and README claim. Unsupported-lock fallbacks need to be limited to the specific OS errors that actually mean "flock is not supported"; all other lock failures must abort the restore.

blocking: Restore's advisory `flock` does not exclude normal SQLite writers, so the documented concurrency guarantee is false

packages/rs-platform-wallet-storage/src/sqlite/backup.rs (line 168)

The README now states that restore holds an exclusive flock(2) "so a concurrent writer can't race the atomic swap", but the crate's own writers opened through open_conn() never take that flock, and ordinary SQLite peers rely on SQLite's file locking rather than this separate advisory lock. A second process can therefore keep writing the wallet database while restore_from() stages the replacement and unlinks -wal/-shm, even when try_lock_exclusive() succeeds. That makes the advertised safety guarantee false under multi-process use and leaves a real data-loss window around the pre-restore backup and final rename.

blocking: `delete_wallet()` can return a successful pre-delete backup that omits buffered-only wallet state

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 383)

delete_wallet_inner() now drains the in-memory changeset first and treats had_buffered as enough to prove the wallet exists, but the pre-delete backup is still taken only from SQLite via run_auto_backup(&conn, ...) before the drained changeset is materialized anywhere. If the wallet exists only in the merge buffer, delete_wallet() succeeds, returns backup_path, and then drops the drained changeset after commit. The regression test pre_delete_backup_excludes_buffered_writes() intentionally demonstrates this behavior, which means the API's "safe-by-default" pre-delete backup cannot actually restore the deleted wallet state.

blocking: `delete_wallet()` takes its rollback backup before any cross-process exclusion, so concurrent commits can be deleted without landing in the backup

packages/rs-platform-wallet-storage/src/sqlite/persister.rs (line 420)

The only serialization here is the in-process Mutex<Connection>. run_auto_backup(&conn, ...) runs before conn.transaction()?, so another process with its own SQLite connection can commit rows for the same wallet after the backup snapshot finishes but before this delete transaction acquires the write lock and cascades the delete. Those rows are then removed from the live database by the delete, but they were never captured in pre-delete-*.db, so the documented rollback safety guarantee is false under multi-process use.

suggestion: `load()` accepts a host-owned wallet-list allocation without requiring the paired free callback

packages/rs-platform-wallet-ffi/src/persistence.rs (line 1284)

The callback contract says on_load_wallet_list_fn returns a host-allocated WalletRestoreEntryFFI array that Rust frees through on_load_wallet_list_free_fn, but FFIPersister::load() only checks whether the loader callback exists. If a consumer wires on_load_wallet_list_fn without on_load_wallet_list_free_fn, LoadGuard is created with free_fn: None and every successful restore leaks the host allocation. This is not an intentional pattern in this file: the shielded loaders already reject half-wired (load, free) callback pairs for exactly the same reason.

suggestion: `on_get_core_tx_record_fn` can hand Rust an owned tx-bytes buffer without requiring the paired free callback

packages/rs-platform-wallet-ffi/src/persistence.rs (line 1572)

The tx-record callback contract says the host allocates out_tx_bytes and Rust returns that buffer through on_get_core_tx_record_free_fn, but get_core_tx_record() never validates that the free callback is present before accepting a non-null tx_bytes_ptr. TxBytesGuard only frees when on_get_core_tx_record_free_fn is Some(...), so a half-wired consumer leaks one allocation per successful lookup. As with the wallet-list loader, this crate already treats half-wired load/free pairs as an error elsewhere, so this callback should enforce the same contract.

🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.

- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:187-199: `restore_from()` treats every non-`WouldBlock` lock failure as "locking unsupported" and proceeds unlocked
  The restore path only handles `WouldBlock` as a hard error. Every other `try_lock_exclusive()` failure falls into `Err(_)`, logs "advisory lock unsupported on this filesystem", and continues with the destructive swap. That branch is too broad: real lock-acquisition failures such as permission errors or I/O failures are silently downgraded into an unsupported-filesystem warning, so the restore can overwrite the destination without the protection the code and README claim. Unsupported-lock fallbacks need to be limited to the specific OS errors that actually mean "flock is not supported"; all other lock failures must abort the restore.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:168-203: Restore's advisory `flock` does not exclude normal SQLite writers, so the documented concurrency guarantee is false
  The README now states that `restore` holds an exclusive `flock(2)` "so a concurrent writer can't race the atomic swap", but the crate's own writers opened through `open_conn()` never take that flock, and ordinary SQLite peers rely on SQLite's file locking rather than this separate advisory lock. A second process can therefore keep writing the wallet database while `restore_from()` stages the replacement and unlinks `-wal`/`-shm`, even when `try_lock_exclusive()` succeeds. That makes the advertised safety guarantee false under multi-process use and leaves a real data-loss window around the pre-restore backup and final rename.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:383-429: `delete_wallet()` can return a successful pre-delete backup that omits buffered-only wallet state
  `delete_wallet_inner()` now drains the in-memory changeset first and treats `had_buffered` as enough to prove the wallet exists, but the pre-delete backup is still taken only from SQLite via `run_auto_backup(&conn, ...)` before the drained changeset is materialized anywhere. If the wallet exists only in the merge buffer, `delete_wallet()` succeeds, returns `backup_path`, and then drops the drained changeset after commit. The regression test `pre_delete_backup_excludes_buffered_writes()` intentionally demonstrates this behavior, which means the API's "safe-by-default" pre-delete backup cannot actually restore the deleted wallet state.
- [BLOCKING] In `packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:420-447: `delete_wallet()` takes its rollback backup before any cross-process exclusion, so concurrent commits can be deleted without landing in the backup
  The only serialization here is the in-process `Mutex<Connection>`. `run_auto_backup(&conn, ...)` runs before `conn.transaction()?`, so another process with its own SQLite connection can commit rows for the same wallet after the backup snapshot finishes but before this delete transaction acquires the write lock and cascades the delete. Those rows are then removed from the live database by the delete, but they were never captured in `pre-delete-*.db`, so the documented rollback safety guarantee is false under multi-process use.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:1284-1303: `load()` accepts a host-owned wallet-list allocation without requiring the paired free callback
  The callback contract says `on_load_wallet_list_fn` returns a host-allocated `WalletRestoreEntryFFI` array that Rust frees through `on_load_wallet_list_free_fn`, but `FFIPersister::load()` only checks whether the loader callback exists. If a consumer wires `on_load_wallet_list_fn` without `on_load_wallet_list_free_fn`, `LoadGuard` is created with `free_fn: None` and every successful restore leaks the host allocation. This is not an intentional pattern in this file: the shielded loaders already reject half-wired `(load, free)` callback pairs for exactly the same reason.
- [SUGGESTION] In `packages/rs-platform-wallet-ffi/src/persistence.rs`:1572-1605: `on_get_core_tx_record_fn` can hand Rust an owned tx-bytes buffer without requiring the paired free callback
  The tx-record callback contract says the host allocates `out_tx_bytes` and Rust returns that buffer through `on_get_core_tx_record_free_fn`, but `get_core_tx_record()` never validates that the free callback is present before accepting a non-null `tx_bytes_ptr`. `TxBytesGuard` only frees when `on_get_core_tx_record_free_fn` is `Some(...)`, so a half-wired consumer leaks one allocation per successful lookup. As with the wallet-list loader, this crate already treats half-wired load/free pairs as an error elsewhere, so this callback should enforce the same contract.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Introduces a reusable, workspace-integrated SQLite persistence backend for platform-wallet by adding a new platform-wallet-storage crate, and adds an opt-in serde feature to platform-wallet so changesets (and related scalar wallet types) can be serialized for durable storage.

Changes:

  • Added platform-wallet-storage crate with SQLite schema/migrations, buffering + atomic flush, backup/restore tooling, and extensive hardening tests.
  • Added platform-wallet/serde feature and gated Serialize/Deserialize derives across changeset and wallet scalar types, with adapters for non-serde upstream types.
  • Wired the new crate into the workspace build (Cargo members, Docker build contexts, and CI workflows).

Reviewed changes

Copilot reviewed 67 out of 68 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
packages/rs-platform-wallet/src/wallet/shielded/store.rs Adds optional serde derives for shielded store IDs/notes.
packages/rs-platform-wallet/src/wallet/identity/types/key_storage.rs Adds optional serde derives for identity scalar types.
packages/rs-platform-wallet/src/wallet/identity/types/dashpay/profile.rs Adds optional serde derives for DashPay profile model.
packages/rs-platform-wallet/src/wallet/identity/types/dashpay/payment.rs Adds optional serde derives for DashPay payment types.
packages/rs-platform-wallet/src/wallet/identity/types/dashpay/established_contact.rs Adds optional serde derives for established contact type.
packages/rs-platform-wallet/src/wallet/identity/types/dashpay/contact_request.rs Adds optional serde derives for contact request type.
packages/rs-platform-wallet/src/wallet/identity/types/block_time.rs Adds optional serde derives for block-time struct.
packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs Adds optional serde derives for asset-lock status enum.
packages/rs-platform-wallet/src/changeset/traits.rs Documents flush error contract (updated wording needed to match API).
packages/rs-platform-wallet/src/changeset/shielded_changeset.rs Adds optional serde derives for shielded changeset.
packages/rs-platform-wallet/src/changeset/serde_adapters.rs New serde with adapters for upstream non-serde types.
packages/rs-platform-wallet/src/changeset/mod.rs Gates serde adapters module behind serde feature.
packages/rs-platform-wallet/src/changeset/changeset.rs Adds optional serde derives across changeset types; uses adapters/skips for non-serde fields.
packages/rs-platform-wallet/Cargo.toml Adds optional serde feature to enable derives (off by default).
packages/rs-platform-wallet-storage/tests/sqlite_restore_staged_validation.rs Regression tests for restore staging validation/TOCTOU closure.
packages/rs-platform-wallet-storage/tests/sqlite_permissions.rs Tests secure permissions on DB + WAL/SHM sidecars (unix).
packages/rs-platform-wallet-storage/tests/sqlite_open_version_gate.rs Tests open-time forward-schema rejection.
packages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rs Tests integrity_check before migration on existing DB.
packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs Tests migration embedding, reach, and drift/fingerprint behavior.
packages/rs-platform-wallet-storage/tests/sqlite_hardening_3625.rs Hardening regression suite (FKs, mismatch guards, overflow guards, load gates).
packages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rs Tests FK enforcement and cascade/set-null behavior.
packages/rs-platform-wallet-storage/tests/sqlite_error_classification.rs Exhaustiveness tests for error classification + boundary mapping.
packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs Tests delete semantics and buffer restore on failure + concurrency race.
packages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rs Tests delete reconciles in-memory buffer vs persisted state.
packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs Compile-time checks (object safety, prepare_cached policy, upstream shape invariants).
packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rs CLI smoke tests for basic commands and usage errors.
packages/rs-platform-wallet-storage/tests/sqlite_backup_restore.rs Backup/restore/retention tests and failure-mode assertions.
packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs Auto-backup behavior tests and failure surfacing.
packages/rs-platform-wallet-storage/tests/secrets_scan.rs Scan schema/migrations for forbidden secret-material tokens.
packages/rs-platform-wallet-storage/tests/common/mod.rs Shared integration test helpers for persister setup/probing.
packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs Safe integer cast helpers to avoid i64/u64 truncation/wrap.
packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs Unix chmod helper to enforce 0o600 on DB and sidecars.
packages/rs-platform-wallet-storage/src/sqlite/util/mod.rs Exposes internal utility modules.
packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs Writer/reader helpers for wallet metadata table.
packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs Writer for token balances table with safe casts.
packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs Writers/readers for platform address state + sync watermarks.
packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs Schema module index + per-wallet table list used for delete/inspect.
packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs Identity public key storage with special wire-shape encoding.
packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs Identity writer + readers for reconstruction helpers.
packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rs Writers for DashPay profiles and payment overlays.
packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs Core wallet writers/readers (transactions, utxos, derived addresses, sync state).
packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs Contact tables writers + test-only hardened reader.
packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs Central bincode-serde blob codec with size cap + strict trailing-byte check.
packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs Asset-lock writer + hardened reader with typed-vs-blob cross-check.
packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs Writers + stable labels for account registration/address pool persistence.
packages/rs-platform-wallet-storage/src/sqlite/mod.rs SQLite module wiring + public re-exports.
packages/rs-platform-wallet-storage/src/sqlite/migrations.rs Migration runner + schema version gating + fingerprinting helpers.
packages/rs-platform-wallet-storage/src/sqlite/conn.rs Centralized connection open + FK enforcement read-back.
packages/rs-platform-wallet-storage/src/sqlite/config.rs Persister config (flush mode, journal, synchronous, auto-backup dir).
packages/rs-platform-wallet-storage/src/sqlite/buffer.rs In-memory per-wallet changeset buffer with take/restore semantics.
packages/rs-platform-wallet-storage/src/lib.rs Crate root module layout + convenience re-exports + compile-time checks.
packages/rs-platform-wallet-storage/SECRETS.md Documents private-key boundary and planned secrets submodule.
packages/rs-platform-wallet-storage/README.md Usage docs for library and CLI + operational notes.
packages/rs-platform-wallet-storage/migrations/V001__initial.rs Initial SQLite schema (tables, FKs, trigger).
packages/rs-platform-wallet-storage/Cargo.toml New crate manifest, deps, and feature gating.
packages/rs-platform-wallet-storage/build.rs Ensures changes under migrations/ trigger rebuilds.
Dockerfile Includes new crate in Docker build contexts.
Cargo.toml Adds new crate to workspace members.
Cargo.lock Adds new dependencies for the new crate + its tests.
.github/workflows/tests-rs-workspace.yml Adds new crate to Rust workspace test shards.
.github/workflows/pr.yml Allows PR scope for the new wallet-storage area.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +91 to +98
```text
platform-wallet-storage --db <path> migrate [--no-auto-backup]
platform-wallet-storage --db <path> backup --out <dir-or-file>
platform-wallet-storage --db <path> restore --from <backup.db> --yes
platform-wallet-storage --db <path> prune --in <dir> [--keep-last N] [--max-age 30d]
platform-wallet-storage --db <path> inspect [--wallet-id <hex>] [--format text|tsv|json]
platform-wallet-storage --db <path> delete-wallet --wallet-id <hex> --yes [--no-auto-backup]
```
Comment on lines +100 to +104
Destructive subcommands (`restore`, `delete-wallet`) REQUIRE `--yes`
— invoking them without it exits 2 with a usage error. `--no-auto-backup`
opts out of the pre-migration / pre-delete auto-backup respectively;
the library API has no equivalent opt-out (it routes to
[`SqlitePersister::delete_wallet_skip_backup`] internally).
Comment on lines +18 to +24
[dependencies]
# Cross-cutting deps (always on)
platform-wallet = { path = "../rs-platform-wallet", features = ["serde"] }
serde = { version = "1", features = ["derive"] }
thiserror = "1"
tracing = "0.1"
hex = "0.4"
Comment on lines +138 to +155
/// Implementations classify failures along a two-axis contract:
///
/// - **Transient** (`PersistenceError::backend(..)` whose source
/// carries `is_transient() == true` — for the canonical SQLite
/// backend that's `SQLITE_BUSY` / `SQLITE_LOCKED`, and as of
/// ATOM-008 also the I/O-class codes `SQLITE_FULL` /
/// `SQLITE_IOERR` / `SQLITE_NOMEM`): the buffered changeset is
/// preserved (re-merged via the buffer's `restore` path so any
/// `store` that landed during the failed flush wins on LWW
/// fields), and the caller MAY retry with exponential backoff.
/// - **Fatal** (everything else — schema corruption, logic bugs,
/// integrity violations): the buffer is dropped, the staged
/// changeset is gone, and the backend logs a structured
/// `tracing::error!`. The caller MUST NOT retry — the data is
/// not recoverable through this trait.
///
/// [`PersistenceError::LockPoisoned`] is fatal but distinguished
/// at the variant level so callers can pattern-match on it.
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 6

🧹 Nitpick comments (3)
packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs (1)

92-97: 💤 Low value

Clarify SafeCastTarget::U64 for i64u32 conversions

SafeCastTarget only has I64 and U64, and other i64u32 reads in this crate also use SafeCastTarget::U64, so U64 appears to be a convention meaning “cast to an unsigned type” rather than “the destination is specifically u64”. Consider renaming/rewording SafeCastTarget::U64 (or the IntegerOverflow error text) to reflect this intent and avoid operator confusion.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs` around
lines 92 - 97, The enum variant SafeCastTarget::U64 is misleading for i64→u32
conversions; update either the enum or the error message to clarify it denotes
"unsigned target" rather than specifically u64: rename SafeCastTarget::U64 to
something like SafeCastTarget::Unsigned (or add a new variant Unsigned) and
update all usages (including the IntegerOverflow construction in asset_locks.rs
where u32::try_from(...) maps to WalletStorageError::IntegerOverflow { field:
"asset_locks.account_index", value: account_index as u64, target: ... }) or
alternatively change the IntegerOverflow error text to reference "unsigned"
instead of "u64" to remove ambiguity; ensure you update all other crate sites
that use SafeCastTarget::U64 to the new name to keep compilation consistent.
packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs (1)

70-74: 💤 Low value

Minor: SafeCastTarget mismatch in error metadata.

The conversion is i64u32 but the error reports target: SafeCastTarget::U64. This doesn't affect functionality but could confuse debugging. Consider using a U32 target variant if available, or updating the error message context.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs` around
lines 70 - 74, The IntegerOverflow error raised during the i64→u32 conversion in
wallet_meta.rs (the u32::try_from(height) call) incorrectly sets target:
SafeCastTarget::U64; change the error metadata to the appropriate variant (e.g.,
SafeCastTarget::U32) or otherwise adjust the error context string so it
correctly reflects the intended target type for
WalletStorageError::IntegerOverflow raised around the variable height.
packages/rs-platform-wallet-storage/src/sqlite/persister.rs (1)

1061-1071: 💤 Low value

Potential truncation when casting schema version from i64 to i32.

The current_schema_version function casts i64 to i32 without bounds checking. While schema versions are unlikely to exceed i32::MAX in practice, this silent truncation could mask issues if the database were corrupted or tampered with.

♻️ Suggested defensive cast
 fn current_schema_version(conn: &Connection) -> Result<Option<i32>, WalletStorageError> {
     let row = conn
         .query_row(
             "SELECT MAX(version) FROM refinery_schema_history",
             [],
             |row| row.get::<_, Option<i64>>(0),
         )
         .optional()?
         .flatten();
-    Ok(row.map(|v| v as i32))
+    Ok(row.map(|v| i32::try_from(v).unwrap_or(i32::MAX)))
 }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs` around lines
1061 - 1071, current_schema_version currently casts the queried Option<i64> to
i32 without bounds checking which can silently truncate a corrupted large value;
modify current_schema_version to validate the i64 -> i32 conversion (e.g., use
try_into or an explicit range check) and return an appropriate
WalletStorageError on overflow instead of performing a blind cast, keeping the
function signature Result<Option<i32>, WalletStorageError> and referencing the
same query_row result handling logic so None stays None and Some(out-of-range)
yields an error.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs`:
- Around line 192-199: The code currently swallows all errors from
try_lock_exclusive() in restore_from and treats them as "advisory lock
unsupported", which hides real failures; change the Err(_) arm to inspect the
error (Err(e)) and only downgrade to None and log the warning when e.kind() ==
std::io::ErrorKind::Unsupported (or the equivalent platform-specific unsupported
indicator); for other errors propagate the error out of restore_from (e.g.,
return Err(e.into()) or map it into the function's error type) so genuine lock
acquisition failures aren't masked. Ensure you reference the
try_lock_exclusive() call and dest_db_path in the updated match to keep the same
warning message for the Unsupported case.
- Around line 73-109: Replace tmp.persist(dest).map_err(|e|
WalletStorageError::Io(e.error))? with a call to
NamedTempFile::persist_noclobber and map the AlreadyExists case to
WalletStorageError::BackupDestinationExists; specifically call
tmp.persist_noclobber(dest) and match its Result, returning Ok on success, and
on Err(e) check if e.error.kind() == std::io::ErrorKind::AlreadyExists then
return Err(WalletStorageError::BackupDestinationExists { path:
dest.to_path_buf() }) else return Err(WalletStorageError::Io(e.error)); keep the
tmp and dest variables and the existing imports.

In `@packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs`:
- Around line 27-35: The code builds sidecar names with to_string_lossy() and
gates set_permissions() behind exists(), which can incorrectly handle non-UTF8
filenames and introduces a TOCTOU race; fix by constructing the sibling filename
using OsString (avoid to_string_lossy) — e.g. take
path.file_name().unwrap().to_os_string(), push the literal suffix ("-wal" /
"-shm") and call path.with_file_name(that_osstring) to get sibling — then remove
the exists() check and call std::fs::set_permissions(&sibling, perms.clone())
directly, handling errors by ignoring NotFound (std::io::ErrorKind::NotFound)
but returning other errors; apply the same change pattern in the related remove
logic in backup.rs where "-wal"/"-shm" are handled.

In `@packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs`:
- Line 139: The test currently ignores the result of persister.delete_wallet(w)
which can hide unexpected failures; update the test in sqlite_delete_wallet.rs
to capture the Result returned by persister.delete_wallet(w) and assert it is Ok
(or explicitly match and panic/log on Err) before proceeding to the later
assertion—reference the persister.delete_wallet(...) call and ensure the test
either .unwrap()s or uses assert!(result.is_ok(), "...") so any unexpected error
fails the test rather than causing a vacuous pass.
- Line 156: The test currently uses .unwrap_or(0) which hides any SQL/query
errors; replace that call so failures surface: either propagate the Result (use
? if the test returns Result) or call .expect("failed to query row count for
<table_name>") / .unwrap() with a clear message instead of unwrap_or(0), so any
missing table or query error fails the test and shows the underlying error;
update the occurrence of unwrap_or(0) in the sqlite_delete_wallet test
accordingly.

In `@packages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rs`:
- Line 28: The assertion currently checks `len > 4096` but the test then calls
`read_exact` for 4096 bytes starting at offset 4096 (bytes 4096..8192), so
update the check on the `len` variable to require the file to be at least 8192
bytes (e.g., `len >= 8192` or `len > 8191`) to guarantee the subsequent
`read_exact` won't panic; change the assertion referring to `len` accordingly in
the test.

---

Nitpick comments:
In `@packages/rs-platform-wallet-storage/src/sqlite/persister.rs`:
- Around line 1061-1071: current_schema_version currently casts the queried
Option<i64> to i32 without bounds checking which can silently truncate a
corrupted large value; modify current_schema_version to validate the i64 -> i32
conversion (e.g., use try_into or an explicit range check) and return an
appropriate WalletStorageError on overflow instead of performing a blind cast,
keeping the function signature Result<Option<i32>, WalletStorageError> and
referencing the same query_row result handling logic so None stays None and
Some(out-of-range) yields an error.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs`:
- Around line 92-97: The enum variant SafeCastTarget::U64 is misleading for
i64→u32 conversions; update either the enum or the error message to clarify it
denotes "unsigned target" rather than specifically u64: rename
SafeCastTarget::U64 to something like SafeCastTarget::Unsigned (or add a new
variant Unsigned) and update all usages (including the IntegerOverflow
construction in asset_locks.rs where u32::try_from(...) maps to
WalletStorageError::IntegerOverflow { field: "asset_locks.account_index", value:
account_index as u64, target: ... }) or alternatively change the IntegerOverflow
error text to reference "unsigned" instead of "u64" to remove ambiguity; ensure
you update all other crate sites that use SafeCastTarget::U64 to the new name to
keep compilation consistent.

In `@packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs`:
- Around line 70-74: The IntegerOverflow error raised during the i64→u32
conversion in wallet_meta.rs (the u32::try_from(height) call) incorrectly sets
target: SafeCastTarget::U64; change the error metadata to the appropriate
variant (e.g., SafeCastTarget::U32) or otherwise adjust the error context string
so it correctly reflects the intended target type for
WalletStorageError::IntegerOverflow raised around the variable height.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 38431db9-73ae-4205-9dd8-464e2edb9c8c

📥 Commits

Reviewing files that changed from the base of the PR and between 9f3bfa3 and fe01634.

⛔ Files ignored due to path filters (1)
  • Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (67)
  • .github/workflows/pr.yml
  • .github/workflows/tests-rs-workspace.yml
  • Cargo.toml
  • Dockerfile
  • packages/rs-platform-wallet-storage/Cargo.toml
  • packages/rs-platform-wallet-storage/README.md
  • packages/rs-platform-wallet-storage/SECRETS.md
  • packages/rs-platform-wallet-storage/build.rs
  • packages/rs-platform-wallet-storage/migrations/V001__initial.rs
  • packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs
  • packages/rs-platform-wallet-storage/src/lib.rs
  • packages/rs-platform-wallet-storage/src/sqlite/backup.rs
  • packages/rs-platform-wallet-storage/src/sqlite/buffer.rs
  • packages/rs-platform-wallet-storage/src/sqlite/config.rs
  • packages/rs-platform-wallet-storage/src/sqlite/conn.rs
  • packages/rs-platform-wallet-storage/src/sqlite/error.rs
  • packages/rs-platform-wallet-storage/src/sqlite/migrations.rs
  • packages/rs-platform-wallet-storage/src/sqlite/mod.rs
  • packages/rs-platform-wallet-storage/src/sqlite/persister.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/accounts.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/asset_locks.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/blob.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/contacts.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/core_state.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/dashpay.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/identities.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/identity_keys.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/mod.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/platform_addrs.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/token_balances.rs
  • packages/rs-platform-wallet-storage/src/sqlite/schema/wallet_meta.rs
  • packages/rs-platform-wallet-storage/src/sqlite/util/mod.rs
  • packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs
  • packages/rs-platform-wallet-storage/src/sqlite/util/safe_cast.rs
  • packages/rs-platform-wallet-storage/tests/common/mod.rs
  • packages/rs-platform-wallet-storage/tests/secrets_scan.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_auto_backup.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_backup_restore.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_buffer_semantics.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_cli_smoke.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_compile_time.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_delete_buffer_reconcile.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_error_classification.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_foreign_keys.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_hardening_3625.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_load_reconstruction.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_migrations.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_open_version_gate.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_permissions.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_persist_roundtrip.rs
  • packages/rs-platform-wallet-storage/tests/sqlite_restore_staged_validation.rs
  • packages/rs-platform-wallet/Cargo.toml
  • packages/rs-platform-wallet/src/changeset/changeset.rs
  • packages/rs-platform-wallet/src/changeset/mod.rs
  • packages/rs-platform-wallet/src/changeset/serde_adapters.rs
  • packages/rs-platform-wallet/src/changeset/shielded_changeset.rs
  • packages/rs-platform-wallet/src/changeset/traits.rs
  • packages/rs-platform-wallet/src/wallet/asset_lock/tracked.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/block_time.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/dashpay/contact_request.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/dashpay/established_contact.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/dashpay/payment.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/dashpay/profile.rs
  • packages/rs-platform-wallet/src/wallet/identity/types/key_storage.rs
  • packages/rs-platform-wallet/src/wallet/shielded/store.rs

Comment on lines +73 to +109
if dest.exists() {
return Err(WalletStorageError::BackupDestinationExists {
path: dest.to_path_buf(),
});
}

// Stage the backup into an unguessable temp file in the same
// directory. Same-FS guarantee makes `persist` an atomic rename.
let parent = dest.parent().unwrap_or(Path::new("."));
let tmp = tempfile::NamedTempFile::new_in(parent)?;
// SEC-011: tighten the temp's mode to 0o600 BEFORE persist so the
// destination inherits owner-only permissions via the atomic
// rename. Running chmod after persist would leave a brief
// umask-default window where the destination is observable.
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
tmp.as_file()
.set_permissions(std::fs::Permissions::from_mode(0o600))?;
}

// Page-stepping copy against the temp. The dest Connection has to
// own its own file handle; rusqlite opens it from a path.
let mut backup_conn =
crate::sqlite::conn::open_conn(tmp.path(), crate::sqlite::conn::Access::ReadWrite)?;
{
let backup = Backup::new(src, &mut backup_conn)?;
// 100 pages × 4 KiB = 400 KiB per step on default SQLite page size.
backup.run_to_completion(100, Duration::from_millis(5), None)?;
}
// Close the backup Connection before persisting so SQLite flushes
// its own WAL/SHM siblings against the temp path — those go away
// with the rename since `persist` atomically renames the temp file.
drop(backup_conn);

tmp.persist(dest)
.map_err(|e| WalletStorageError::Io(e.error))?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Locate the target file and print the relevant section
FILE="packages/rs-platform-wallet-storage/src/sqlite/backup.rs"
echo "== $FILE =="
nl -ba "$FILE" | sed -n '1,220p'

# Print WalletStorageError definition(s)
echo
rg -n "enum WalletStorageError|WalletStorageError::BackupDestinationExists" -S packages/rs-platform-wallet-storage/src | head -n 50

# Show the exact error variant definition context
ERRFILE=$(rg -n "BackupDestinationExists" -S packages/rs-platform-wallet-storage/src --files-with-matches | head -n 1)
echo
echo "== error file: $ERRFILE =="
nl -ba "$ERRFILE" | sed -n '1,220p'

# Check how tempfile persist is used in this crate
echo
echo "== tempfile persist usage in crate =="
rg -n "persist(_noclobber)?\(" packages/rs-platform-wallet-storage/src/sqlite/backup.rs packages/rs-platform-wallet-storage/src/sqlite -S

# Also check other occurrences across repo for persist_noclobber
echo
echo "== persist_noclobber usages across repo =="
rg -n "persist_noclobber\\(" -S packages | head -n 50

Repository: dashpay/platform

Length of output: 184


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="packages/rs-platform-wallet-storage/src/sqlite/backup.rs"
if [ ! -f "$FILE" ]; then
  echo "Missing $FILE"
  exit 1
fi

echo "== snippet (backup.rs) =="
cat -n "$FILE" | sed -n '50,160p'

echo
echo "== WalletStorageError definitions (search) =="
rg -n "enum WalletStorageError|BackupDestinationExists" packages/rs-platform-wallet-storage/src/sqlite -S
rg -n "enum WalletStorageError|BackupDestinationExists" packages/rs-platform-wallet-storage/src -S

echo
echo "== persist/persist_noclobber usage in sqlite/backup.rs =="
rg -n "persist_noclobber\\(|\\.persist\\(" "$FILE" -S

echo
echo "== persist_noclobber usage across packages =="
rg -n "persist_noclobber\\(" packages -S

Repository: dashpay/platform

Length of output: 7960


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Paths
ERR="packages/rs-platform-wallet-storage/src/sqlite/error.rs"
PERS="packages/rs-platform-wallet-storage/src/sqlite/persister.rs"
CLI="packages/rs-platform-wallet-storage/src/bin/platform-wallet-storage.rs"
BK="packages/rs-platform-wallet-storage/src/sqlite/backup.rs"

echo "== WalletStorageError enum (BackupDestinationExists) =="
cat -n "$ERR" | sed -n '1,260p'

echo
echo "== Where BackupDestinationExists is used (persister.rs) =="
cat -n "$PERS" | sed -n '200,320p'

echo
echo "== CLI mapping for BackupDestinationExists =="
cat -n "$CLI" | sed -n '250,340p'

echo
echo "== backup.rs restore_from persist path (to see how collisions are handled) =="
# show around second persist at ~273 from earlier rg output
cat -n "$BK" | sed -n '240,320p'

Repository: dashpay/platform

Length of output: 26142


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Show where tempfile is declared (version)
echo "== tempfile dependency declarations =="
rg -n "tempfile\s*=" -S Cargo.toml packages -g'Cargo.toml'

# If workspace Cargo.lock exists, show tempfile entry/version
if [ -f Cargo.lock ]; then
  echo
  echo "== tempfile in Cargo.lock (version pinned by workspace) =="
  rg -n "name = \"tempfile\"|tempfile " Cargo.lock | head -n 40
fi

# Also capture the exact lines for apply_secure_permissions call site
FILE="packages/rs-platform-wallet-storage/src/sqlite/backup.rs"
echo
echo "== apply_secure_permissions invocation around run_to =="
cat -n "$FILE" | sed -n '60,140p'

Repository: dashpay/platform

Length of output: 4879


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Extract exact tempfile version from Cargo.lock
rg -n "^name = \"tempfile\"$" -n Cargo.lock
line=$(rg -n "^name = \"tempfile\"$" Cargo.lock | head -n1 | cut -d: -f1)
echo "tempfile name line: $line"
start=$((line-2))
end=$((line+10))
sed -n "${start},${end}p" Cargo.lock

Repository: dashpay/platform

Length of output: 407


🌐 Web query:

tempfile 3.27.0 NamedTempFile persist overwrite existing destination persist_noclobber

💡 Result:

For tempfile::NamedTempFile v3.27.0: 1) NamedTempFile::persist(new_path) - If a file exists at new_path, persist atomically replaces it.[1][2] 2) NamedTempFile::persist_noclobber(new_path) - Persist the temporary file at new_path only if no file exists there; if a file exists, it fails (returns Err / returns self in PersistError).[2] - It will never overwrite the existing target, but it is “not atomic” on all platforms; it may leave the original link to the temporary file behind in some failure/crash scenarios (so you may end up with multiple hard links pointing to the same underlying file).[2] So, your phrase “persist overwrite existing destination persist_noclobber” maps to this behavior: - persist = overwrites existing destination (atomically replace) when possible.[1][2] - persist_noclobber = does NOT overwrite; it fails if the destination already exists.[2] If you’re persisting by path rather than from a NamedTempFile handle, TempPath::persist has the same “atomic replace if exists” semantics, while TempPath::persist_noclobber fails if the destination exists.[3][4]

Citations:


🌐 Web query:

tempfile 3.27.0 PersistError persist_noclobber error kind AlreadyExists

💡 Result:

In tempfile (Rust) 3.27.x, PersistError and persist_noclobber are intended to prevent overwriting an existing destination path. If persist_noclobber is called and the destination path already exists, the operation fails and returns PersistError (wrapping the underlying IO error) rather than replacing the file. [1][2] How this relates to “error kind AlreadyExists” - NamedTempFile::persist_noclobber “Persist[s] the temporary file … if and only if no file exists there.” If a file exists at the target path, “fail[s]” and returns self in a resulting PersistError. [1] - The underlying failure is an IO error (PersistError has an error field for the underlying IO error). [2] - Separately, tempfile’s APIs that create/open files treat std::io::ErrorKind::AlreadyExists as a signal to retry/choose another randomized path; tempfile documents this behavior in its Builder logic. [3] So, when you see “PersistError persist_noclobber error kind AlreadyExists”, it generally means: 1) persist_noclobber attempted to persist to a destination path, 2) that destination path already existed, 3) the underlying IO layer reported AlreadyExists, 4) tempfile wrapped that IO error inside PersistError. [1][2] If you share the exact Rust snippet and the full error/debug output, I can point to the precise call site and confirm whether it’s the destination-file-exists case (expected) vs. some other AlreadyExists condition.

Citations:


Fix TOCTOU overwrite by replacing persist(dest) with persist_noclobber(dest)

dest.exists() is a TOCTOU race: another process can create dest after the check and before tmp.persist(dest), and NamedTempFile::persist will replace an existing destination. This breaks the “no overwrite” contract and currently maps collisions to WalletStorageError::Io(...) instead of BackupDestinationExists.

🔧 Suggested fix (preserve typed error on collisions)
-    if dest.exists() {
-        return Err(WalletStorageError::BackupDestinationExists {
-            path: dest.to_path_buf(),
-        });
-    }
@@
-    tmp.persist(dest)
-        .map_err(|e| WalletStorageError::Io(e.error))?;
+    tmp.persist_noclobber(dest).map_err(|e| {
+        if e.error.kind() == std::io::ErrorKind::AlreadyExists {
+            WalletStorageError::BackupDestinationExists {
+                path: dest.to_path_buf(),
+            }
+        } else {
+            WalletStorageError::Io(e.error)
+        }
+    })?;
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if dest.exists() {
return Err(WalletStorageError::BackupDestinationExists {
path: dest.to_path_buf(),
});
}
// Stage the backup into an unguessable temp file in the same
// directory. Same-FS guarantee makes `persist` an atomic rename.
let parent = dest.parent().unwrap_or(Path::new("."));
let tmp = tempfile::NamedTempFile::new_in(parent)?;
// SEC-011: tighten the temp's mode to 0o600 BEFORE persist so the
// destination inherits owner-only permissions via the atomic
// rename. Running chmod after persist would leave a brief
// umask-default window where the destination is observable.
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
tmp.as_file()
.set_permissions(std::fs::Permissions::from_mode(0o600))?;
}
// Page-stepping copy against the temp. The dest Connection has to
// own its own file handle; rusqlite opens it from a path.
let mut backup_conn =
crate::sqlite::conn::open_conn(tmp.path(), crate::sqlite::conn::Access::ReadWrite)?;
{
let backup = Backup::new(src, &mut backup_conn)?;
// 100 pages × 4 KiB = 400 KiB per step on default SQLite page size.
backup.run_to_completion(100, Duration::from_millis(5), None)?;
}
// Close the backup Connection before persisting so SQLite flushes
// its own WAL/SHM siblings against the temp path — those go away
// with the rename since `persist` atomically renames the temp file.
drop(backup_conn);
tmp.persist(dest)
.map_err(|e| WalletStorageError::Io(e.error))?;
// Stage the backup into an unguessable temp file in the same
// directory. Same-FS guarantee makes `persist` an atomic rename.
let parent = dest.parent().unwrap_or(Path::new("."));
let tmp = tempfile::NamedTempFile::new_in(parent)?;
// SEC-011: tighten the temp's mode to 0o600 BEFORE persist so the
// destination inherits owner-only permissions via the atomic
// rename. Running chmod after persist would leave a brief
// umask-default window where the destination is observable.
#[cfg(unix)]
{
use std::os::unix::fs::PermissionsExt;
tmp.as_file()
.set_permissions(std::fs::Permissions::from_mode(0o600))?;
}
// Page-stepping copy against the temp. The dest Connection has to
// own its own file handle; rusqlite opens it from a path.
let mut backup_conn =
crate::sqlite::conn::open_conn(tmp.path(), crate::sqlite::conn::Access::ReadWrite)?;
{
let backup = Backup::new(src, &mut backup_conn)?;
// 100 pages × 4 KiB = 400 KiB per step on default SQLite page size.
backup.run_to_completion(100, Duration::from_millis(5), None)?;
}
// Close the backup Connection before persisting so SQLite flushes
// its own WAL/SHM siblings against the temp path — those go away
// with the rename since `persist` atomically renames the temp file.
drop(backup_conn);
tmp.persist_noclobber(dest).map_err(|e| {
if e.error.kind() == std::io::ErrorKind::AlreadyExists {
WalletStorageError::BackupDestinationExists {
path: dest.to_path_buf(),
}
} else {
WalletStorageError::Io(e.error)
}
})?;
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs` around lines 73 -
109, Replace tmp.persist(dest).map_err(|e| WalletStorageError::Io(e.error))?
with a call to NamedTempFile::persist_noclobber and map the AlreadyExists case
to WalletStorageError::BackupDestinationExists; specifically call
tmp.persist_noclobber(dest) and match its Result, returning Ok on success, and
on Err(e) check if e.error.kind() == std::io::ErrorKind::AlreadyExists then
return Err(WalletStorageError::BackupDestinationExists { path:
dest.to_path_buf() }) else return Err(WalletStorageError::Io(e.error)); keep the
tmp and dest variables and the existing imports.

Comment on lines +192 to +199
Err(_) => {
tracing::warn!(
target: "platform_wallet_storage",
dest = %dest_db_path.display(),
"advisory lock unsupported on this filesystem; concurrent-writer race possible"
);
None
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="packages/rs-platform-wallet-storage/src/sqlite/backup.rs"

# Show surrounding code for the reported lines
nl -ba "$FILE" | sed -n '150,260p'

# Find any other lock-related handling in the same module/file
rg -n "advisory lock|WouldBlock|Unsupported|lock" "$FILE"

# Show the WalletStorageError definition / Io mapping if present
rg -n "enum WalletStorageError|WalletStorageError::|struct WalletStorageError" packages/rs-platform-wallet-storage/src -S
rg -n "WalletStorageError::Io|Io\\(" packages/rs-platform-wallet-storage/src -S

Repository: dashpay/platform

Length of output: 103


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE="packages/rs-platform-wallet-storage/src/sqlite/backup.rs"

# Print the relevant region (approx. reported lines)
sed -n '170,230p' "$FILE"

# Search for lock/WouldBlock/Unsupported handling in this file
rg -n "advisory lock|WouldBlock|Unsupported|lock" "$FILE" || true

# Locate WalletStorageError definition and its Io mapping
rg -n "enum WalletStorageError|WalletStorageError::|struct WalletStorageError" packages/rs-platform-wallet-storage/src -S
rg -n "WalletStorageError::Io|Io\\(" packages/rs-platform-wallet-storage/src -S

Repository: dashpay/platform

Length of output: 16369


🏁 Script executed:

#!/bin/bash
set -euo pipefail

FILE_BACKUP="packages/rs-platform-wallet-storage/src/sqlite/backup.rs"
FILE_ERROR="packages/rs-platform-wallet-storage/src/sqlite/error.rs"

# Show more context around the lock handling
sed -n '120,230p' "$FILE_BACKUP"

# Find WalletStorageError variants relevant to lock/restore
rg -n "RestoreDestinationLocked|lock|Unsupported|advisory" "$FILE_ERROR"

# Print WalletStorageError enum around its beginning
sed -n '1,220p' "$FILE_ERROR"

Repository: dashpay/platform

Length of output: 15055


Restrict the lock-skip to ErrorKind::Unsupported; propagate other lock acquisition errors.

restore_from currently matches Err(_) for try_lock_exclusive() and logs “advisory lock unsupported…; concurrent-writer race possible”, then continues with _lock = None, masking non-WouldBlock failures as if they were unsupported filesystem issues.

🔧 Suggested fix (only downgrade known “unsupported lock” cases)
-            Err(_) => {
+            Err(e) if e.kind() == std::io::ErrorKind::Unsupported => {
                 tracing::warn!(
                     target: "platform_wallet_storage",
                     dest = %dest_db_path.display(),
                     "advisory lock unsupported on this filesystem; concurrent-writer race possible"
                 );
                 None
             }
+            Err(e) => return Err(WalletStorageError::Io(e)),
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/backup.rs` around lines 192 -
199, The code currently swallows all errors from try_lock_exclusive() in
restore_from and treats them as "advisory lock unsupported", which hides real
failures; change the Err(_) arm to inspect the error (Err(e)) and only downgrade
to None and log the warning when e.kind() == std::io::ErrorKind::Unsupported (or
the equivalent platform-specific unsupported indicator); for other errors
propagate the error out of restore_from (e.g., return Err(e.into()) or map it
into the function's error type) so genuine lock acquisition failures aren't
masked. Ensure you reference the try_lock_exclusive() call and dest_db_path in
the updated match to keep the same warning message for the Unsupported case.

Comment on lines +27 to +35
for ext in ["-wal", "-shm"] {
let sibling = path.with_file_name(format!(
"{}{ext}",
path.file_name()
.map(|s| s.to_string_lossy().to_string())
.unwrap_or_default()
));
if sibling.exists() {
std::fs::set_permissions(&sibling, perms.clone())?;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Verify all potentially lossy sidecar-path constructions in this crate.
rg -n --type rust 'to_string_lossy\(\)\.to_string\(\)|with_file_name\(format!\(' packages/rs-platform-wallet-storage/src/sqlite

Repository: dashpay/platform

Length of output: 556


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Print the exact code around the reported lines in permissions.rs
sed -n '1,120p' packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs | nl -ba | sed -n '20,70p'

# Print the matching section in backup.rs around the lossy sibling construction
sed -n '200,310p' packages/rs-platform-wallet-storage/src/sqlite/backup.rs | nl -ba | sed -n '230,270p'

Repository: dashpay/platform

Length of output: 103


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# permissions.rs: show lines ~20-70 with line numbers
awk 'NR>=20 && NR<=70 {printf "%5d\t%s\n", NR, $0}' packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs

# backup.rs: show lines ~230-270 with line numbers
awk 'NR>=230 && NR<=270 {printf "%5d\t%s\n", NR, $0}' packages/rs-platform-wallet-storage/src/sqlite/backup.rs

Repository: dashpay/platform

Length of output: 2917


Avoid lossy sidecar filename construction for chmod targets (and remove the exists() TOCTOU gate).

In packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs, the -wal/-shm sibling path is built using path.file_name().map(|s| s.to_string_lossy().to_string()), which can change non-UTF-8 filenames and therefore chmod the wrong/missing sidecar. The if sibling.exists() { set_permissions(...) } check introduces a TOCTOU window and can still skip hardening.

Same lossy + exists() pattern also appears in packages/rs-platform-wallet-storage/src/sqlite/backup.rs when removing -wal/-shm.

💡 Suggested fix
         for ext in ["-wal", "-shm"] {
-            let sibling = path.with_file_name(format!(
-                "{}{ext}",
-                path.file_name()
-                    .map(|s| s.to_string_lossy().to_string())
-                    .unwrap_or_default()
-            ));
-            if sibling.exists() {
-                std::fs::set_permissions(&sibling, perms.clone())?;
-            }
+            if let Some(file_name) = path.file_name() {
+                let mut sibling_name = file_name.to_os_string();
+                sibling_name.push(ext);
+                let sibling = path.with_file_name(sibling_name);
+                match std::fs::set_permissions(&sibling, perms.clone()) {
+                    Ok(()) => {}
+                    Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
+                    Err(e) => return Err(e.into()),
+                }
+            }
         }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
for ext in ["-wal", "-shm"] {
let sibling = path.with_file_name(format!(
"{}{ext}",
path.file_name()
.map(|s| s.to_string_lossy().to_string())
.unwrap_or_default()
));
if sibling.exists() {
std::fs::set_permissions(&sibling, perms.clone())?;
for ext in ["-wal", "-shm"] {
if let Some(file_name) = path.file_name() {
let mut sibling_name = file_name.to_os_string();
sibling_name.push(ext);
let sibling = path.with_file_name(sibling_name);
match std::fs::set_permissions(&sibling, perms.clone()) {
Ok(()) => {}
Err(e) if e.kind() == std::io::ErrorKind::NotFound => {}
Err(e) => return Err(e.into()),
}
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/src/sqlite/util/permissions.rs` around
lines 27 - 35, The code builds sidecar names with to_string_lossy() and gates
set_permissions() behind exists(), which can incorrectly handle non-UTF8
filenames and introduces a TOCTOU race; fix by constructing the sibling filename
using OsString (avoid to_string_lossy) — e.g. take
path.file_name().unwrap().to_os_string(), push the literal suffix ("-wal" /
"-shm") and call path.with_file_name(that_osstring) to get sibling — then remove
the exists() check and call std::fs::set_permissions(&sibling, perms.clone())
directly, handling errors by ignoring NotFound (std::io::ErrorKind::NotFound)
but returning other errors; apply the same change pattern in the related remove
logic in backup.rs where "-wal"/"-shm" are handled.


// Give the worker a moment to land some racing stores.
std::thread::sleep(std::time::Duration::from_millis(20));
let _ = persister.delete_wallet(w);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Ignoring delete_wallet result may cause vacuous pass.

If delete_wallet fails due to an unexpected error (not the expected race condition), the wallet rows remain and the assertion at Line 157 passes vacuously because no deletion occurred. Consider asserting that delete succeeded or at minimum logging/checking the result.

Proposed fix
-    let _ = persister.delete_wallet(w);
+    persister
+        .delete_wallet(w)
+        .expect("delete_wallet should succeed for concurrent-store test");
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let _ = persister.delete_wallet(w);
persister
.delete_wallet(w)
.expect("delete_wallet should succeed for concurrent-store test");
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs` at line
139, The test currently ignores the result of persister.delete_wallet(w) which
can hide unexpected failures; update the test in sqlite_delete_wallet.rs to
capture the Result returned by persister.delete_wallet(w) and assert it is Ok
(or explicitly match and panic/log on Err) before proceeding to the later
assertion—reference the persister.delete_wallet(...) call and ensure the test
either .unwrap()s or uses assert!(result.is_ok(), "...") so any unexpected error
fails the test rather than causing a vacuous pass.

rusqlite::params![w.as_slice()],
|row| row.get(0),
)
.unwrap_or(0);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

unwrap_or(0) masks potential query errors.

If the table doesn't exist or the query fails for any reason, the test silently treats this as zero rows and passes. This could hide schema regressions or other failures.

Proposed fix
-            .unwrap_or(0);
+            .expect(&format!("query COUNT(*) from {table} should succeed"));
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/tests/sqlite_delete_wallet.rs` at line
156, The test currently uses .unwrap_or(0) which hides any SQL/query errors;
replace that call so failures surface: either propagate the Result (use ? if the
test returns Result) or call .expect("failed to query row count for
<table_name>") / .unwrap() with a clear message instead of unwrap_or(0), so any
missing table or query error fails the test and shows the underlying error;
update the occurrence of unwrap_or(0) in the sqlite_delete_wallet test
accordingly.

.open(path)
.expect("open db for corruption");
let len = f.metadata().unwrap().len();
assert!(len > 4096, "expected at least one full page");
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical | ⚡ Quick win

Fix assertion to match the required file size.

The assertion len > 4096 is insufficient. Lines 30-32 read 4096 bytes starting at offset 4096 (bytes 4096..8192), which requires the file to be at least 8192 bytes. A file between 4097 and 8191 bytes would pass the assertion but cause read_exact to panic.

🐛 Proposed fix
-    assert!(len > 4096, "expected at least one full page");
+    assert!(len >= 8192, "expected at least two full pages (to corrupt page 2)");
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
assert!(len > 4096, "expected at least one full page");
assert!(len >= 8192, "expected at least two full pages (to corrupt page 2)");
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@packages/rs-platform-wallet-storage/tests/sqlite_open_integrity_check.rs` at
line 28, The assertion currently checks `len > 4096` but the test then calls
`read_exact` for 4096 bytes starting at offset 4096 (bytes 4096..8192), so
update the check on the `len` variable to require the file to be at least 8192
bytes (e.g., `len >= 8192` or `len > 8191`) to guarantee the subsequent
`read_exact` won't panic; change the assertion referring to `len` accordingly in
the test.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

8 participants